Fixing pull --beta and refactoring along the way (#1084)

* trying to enable old json formats

* refactor project.repo to project.config

* changeset

* test and refactor in workspace

* make project meta public

* new helper to load workspace file

* standardise workflow.yaml loading

* update new format for openfn.yaml

* fix types

* suppress error logging

* types

* cli: fix config

* fix tests

* track project name in legacy config files

* track project name from app state

* update test

* fix more tests

* fix empty workspace files

Author: josephjclark

.changeset/fine-mammals-juggle.md

@@ -0,0 +1,5 @@
+---
+'@openfn/cli': patch
+---
+
+Update Project dependency

.changeset/fuzzy-colts-invite.md

@@ -0,0 +1,5 @@
+---
+'@openfn/project': patch
+---
+
+Refactor project.repo to project.config

packages/cli/src/checkout/handler.ts

@@ -15,7 +15,7 @@ const checkoutHandler = async (options: CheckoutOptions, logger: Logger) => {
 
   // get the config
   // TODO: try to retain the endpoint for the projects
-  const { project: _, ...config } = workspace.getConfig() ?? {};
+  const { project: _, ...config } = workspace.getConfig() as any;
 
   // get the project
   let switchProject;

packages/cli/src/pull/beta.ts

@@ -3,7 +3,7 @@ import { confirm } from '@inquirer/prompts';
 import path from 'path';
 import fs from 'node:fs/promises';
 import { DeployConfig, getProject } from '@openfn/deploy';
-import Project from '@openfn/project';
+import Project, { Workspace } from '@openfn/project';
 import type { Logger } from '../util/logger';
 import { rimraf } from 'rimraf';
 import { Opts } from '../options';
@@ -37,47 +37,52 @@ export type PullOptionsBeta = Required<
 export async function handler(options: PullOptionsBeta, logger: Logger) {
   const { OPENFN_API_KEY, OPENFN_ENDPOINT } = process.env;
 
-  const config: Partial<Config> = {
+  const cfg: Partial<Config> = {
     apiKey: options.apiKey,
     endpoint: options.endpoint,
   };
 
   if (!options.apiKey && OPENFN_API_KEY) {
     logger.info('Using OPENFN_API_KEY environment variable');
-    config.apiKey = OPENFN_API_KEY;
+    cfg.apiKey = OPENFN_API_KEY;
   }
 
   if (!options.endpoint && OPENFN_ENDPOINT) {
     logger.info('Using OPENFN_ENDPOINT environment variable');
-    config.endpoint = OPENFN_ENDPOINT;
+    cfg.endpoint = OPENFN_ENDPOINT;
   }
 
+  // TODO `path` or `output` ?
+  // I don't think I want to model this as output. deploy is really
+  // designed to run from the working folder
+  // could be projectPath or repoPath too
+  const outputRoot = path.resolve(options.path || '.');
+
+  // TODO is outputRoot the right dir for this?
+  const workspace = new Workspace(outputRoot);
+  const config = workspace.getConfig();
+
   // download the state.json from lightning
-  const { data } = await getProject(config as DeployConfig, options.projectId);
+  const { data } = await getProject(cfg as DeployConfig, options.projectId);
 
   // TODO if the user doesn't specify an env name, prompt for one
   const name = options.env || 'project';
 
   const project = Project.from('state', data, {
-    endpoint: config.endpoint,
+    config,
+    endpoint: cfg.endpoint,
     env: name,
     fetched_at: new Date().toISOString(),
   });
 
-  // TODO `path` or `output` ?
-  // I don't think I want to model this as output. deploy is really
-  // designed to run from the working folder
-  // could be projectPath or repoPath too
-  const outputRoot = path.resolve(options.path || '.');
-
   const projectFileName = project.getIdentifier();
 
   await fs.mkdir(`${outputRoot}/.projects`, { recursive: true });
   let stateOutputPath = `${outputRoot}/.projects/${projectFileName}`;
 
   const workflowsRoot = path.resolve(
     outputRoot,
-    project.repo?.workflowRoot ?? 'workflows'
+    project.config.dirs.workflows ?? 'workflows'
   );
   // Prompt before deleting
   // TODO this is actually the wrong path
@@ -96,7 +101,8 @@ export async function handler(options: PullOptionsBeta, logger: Logger) {
   await rimraf(workflowsRoot);
 
   const state = project?.serialize('state');
-  if (project.repo?.formats.project === 'yaml') {
+
+  if (project.config.formats.project === 'yaml') {
     await fs.writeFile(`${stateOutputPath}.yaml`, state);
   } else {
     await fs.writeFile(

packages/cli/test/checkout/handler.test.ts

@@ -164,7 +164,7 @@ test.serial('checkout: invalid project id', (t) => {
 test.serial('checkout: to a different valid project', async (t) => {
   // before checkout. some-project-name is active and expanded
   const bcheckout = new Workspace('/ws');
-  t.is(bcheckout.getConfig()?.name, 'some-project-name');
+  t.is(bcheckout.projectMeta.name, 'some-project-name');
   t.is(bcheckout.getActiveProject()?.name, 'some-project-name');
 
   await checkoutHandler(
@@ -176,7 +176,7 @@ test.serial('checkout: to a different valid project', async (t) => {
 
   // after checkout. main-project-id is active and expanded
   const acheckout = new Workspace('/ws');
-  t.is(acheckout.getConfig()?.name, 'main-project-id');
+  t.is(acheckout.projectMeta.name, 'main-project-id');
   t.is(acheckout.getActiveProject()?.name, 'main-project-id');
 
   // check if files where well expanded
@@ -189,19 +189,23 @@ test.serial('checkout: to a different valid project', async (t) => {
 test.serial('checkout: same id as active', async (t) => {
   // before checkout. some-project-name is active and expanded
   const bcheckout = new Workspace('/ws');
-  t.is(bcheckout.getConfig()?.name, 'some-project-name');
+  t.is(bcheckout.projectMeta.name, 'some-project-name');
   t.is(bcheckout.getActiveProject()?.name, 'some-project-name');
 
   await checkoutHandler(
-    { command: 'checkout', projectName: 'some-project-name', projectPath: '/ws' },
+    {
+      command: 'checkout',
+      projectName: 'some-project-name',
+      projectPath: '/ws',
+    },
     logger
   );
   const { message } = logger._parse(logger._last);
   t.is(message, 'Expanded project to /ws');
 
   // after checkout. main-project-id is active and expanded
   const acheckout = new Workspace('/ws');
-  t.is(acheckout.getConfig()?.name, 'some-project-name');
+  t.is(acheckout.projectMeta.name, 'some-project-name');
   t.is(acheckout.getActiveProject()?.name, 'some-project-name');
 
   // check if files where well expanded
@@ -214,7 +218,7 @@ test.serial('checkout: same id as active', async (t) => {
 test.serial('checkout: switching to and back between projects', async (t) => {
   // before checkout. some-project-name is active and expanded
   const bcheckout = new Workspace('/ws');
-  t.is(bcheckout.getConfig()?.name, 'some-project-name');
+  t.is(bcheckout.projectMeta.name, 'some-project-name');
   t.is(bcheckout.getActiveProject()?.name, 'some-project-name');
 
   // 1. switch from some-project-name to main-project-id
@@ -227,7 +231,7 @@ test.serial('checkout: switching to and back between projects', async (t) => {
 
   // after checkout. main-project-id is active and expanded
   const acheckout = new Workspace('/ws');
-  t.is(acheckout.getConfig()?.name, 'main-project-id');
+  t.is(acheckout.projectMeta.name, 'main-project-id');
   t.is(acheckout.getActiveProject()?.name, 'main-project-id');
 
   // check if files where well expanded
@@ -238,15 +242,19 @@ test.serial('checkout: switching to and back between projects', async (t) => {
 
   // 2. switch back from main-project-id to some-project-name
   await checkoutHandler(
-    { command: 'checkout', projectName: 'some-project-name', projectPath: '/ws' },
+    {
+      command: 'checkout',
+      projectName: 'some-project-name',
+      projectPath: '/ws',
+    },
     logger
   );
   const { message: lastMsg } = logger._parse(logger._last);
   t.is(lastMsg, 'Expanded project to /ws');
 
   // after checkout. main-project-id is active and expanded
   const fcheckout = new Workspace('/ws');
-  t.is(fcheckout.getConfig()?.name, 'some-project-name');
+  t.is(fcheckout.projectMeta.name, 'some-project-name');
   t.is(fcheckout.getActiveProject()?.name, 'some-project-name');
 
   // check if files where well expanded

packages/cli/test/merge/handler.test.ts

@@ -113,7 +113,7 @@ test('merging into the same project', async (t) => {
 test('merging a different project into checked-out', async (t) => {
   // state of main projects workflow before sandbox is merged in
   const bworkspace = new Workspace('/ws');
-  t.is(bworkspace.getConfig()?.name, 'main-project-id');
+  t.is(bworkspace.projectMeta.name, 'main-project-id');
   t.is(bworkspace.getActiveProject()?.name, 'main-project-id');
   const bprojects = bworkspace.list();
   t.is(bprojects[0].workflows[0].steps.length, 2);
@@ -133,13 +133,13 @@ test('merging a different project into checked-out', async (t) => {
 
   // state of main projects workflow before sandbox is merged in
   const workspace = new Workspace('/ws');
-  t.is(workspace.getConfig()?.name, 'main-project-id');
+  t.is(workspace.projectMeta.name, 'main-project-id');
   t.is(workspace.getActiveProject()?.name, 'main-project-id');
   const projects = workspace.list();
   t.is(projects[0].workflows[0].steps.length, 3);
   t.is(projects[0].workflows[0].steps[1].name, 'Job X');
   t.is(projects[0].workflows[0].steps[1].openfn?.uuid, 'job-a'); // id got retained
-  t.is(projects[0].workflows[0].steps[2].name, 'Job Y'); 
+  t.is(projects[0].workflows[0].steps[2].name, 'Job Y');
   t.is(projects[0].workflows[0].steps[2].openfn?.uuid, 'job-y'); // id not retained - new nod
 
   const { message, level } = logger._parse(logger._last);

packages/project/src/Project.ts

@@ -1,46 +1,24 @@
-import * as l from '@openfn/lexicon';
 import Workflow from './Workflow';
 import * as serializers from './serialize';
-import fromAppState from './parse/from-app-state';
-import fromPath from './parse/from-path';
+import fromAppState, { FromAppStateConfig } from './parse/from-app-state';
+import fromPath, { FromPathConfig } from './parse/from-path';
 // TODO this naming clearly isn't right
 import { parseProject as fromFs, FromFsConfig } from './parse/from-fs';
 import getIdentifier from './util/get-identifier';
 import slugify from './util/slugify';
 import { getUuidForEdge, getUuidForStep } from './util/uuid';
 import { merge, MergeProjectOptions } from './merge/merge-project';
+import { Workspace } from './Workspace';
+import { buildConfig, WorkspaceConfig } from './util/config';
 
 type MergeOptions = {
   force?: boolean;
   workflows?: string[]; // which workflows to include
 };
 
-type FileFormats = 'yaml' | 'json';
-
 const maybeCreateWorkflow = (wf: any) =>
   wf instanceof Workflow ? wf : new Workflow(wf);
 
-export interface OpenfnConfig {
-  name: string;
-  workflowRoot: string;
-  dirs: {
-    workflows: string;
-    projects: string;
-  };
-  formats: {
-    openfn: FileFormats;
-    project: FileFormats;
-    workflow: FileFormats;
-  };
-  project: {
-    projectId: string;
-    endpoint: string;
-    env: string;
-    inserted_at: string;
-    updated_at: string;
-  };
-}
-
 // TODO --------------
 // I think this needs renaming to config
 // and it's part of the workspace technically
@@ -68,16 +46,7 @@ type RepoOptions = {
 
 // TODO maybe use an npm for this, or create  util
 
-const setConfigDefaults = (config: OpenfnConfig = {}) => ({
-  ...config,
-  workflowRoot: config.workflowRoot ?? 'workflows',
-  formats: {
-    // TODO change these maybe
-    openfn: config.formats?.openfn ?? 'yaml',
-    project: config.formats?.project ?? 'yaml',
-    workflow: config.formats?.workflow ?? 'yaml',
-  },
-});
+// TODO this need to be controlled by the workspace
 
 // A single openfn project
 // could be an app project or a checked out fs
@@ -106,10 +75,9 @@ export class Project {
   // this contains meta about the connected openfn project
   openfn?: l.ProjectConfig;
 
-  // workspace-wide configuration options
-  // these should be shared across projects
-  // and saved to an openfn.yaml file
-  repo?: Required<RepoOptions>;
+  workspace?: Workspace;
+
+  config: WorkspaceConfig;
 
   // load a project from a state file (project.json)
   // or from a path (the file system)
@@ -128,12 +96,12 @@ export class Project {
   static from(
     type: 'path',
     data: string,
-    options?: { config?: Partial<OpenfnConfig> }
+    options?: { config?: FromPathConfig }
   ): Project;
   static from(
     type: 'state' | 'path' | 'fs',
     data: any,
-    options: Partial<l.ProjectConfig> = {}
+    options: FromAppStateConfig = {}
   ): Project {
     if (type === 'state') {
       return fromAppState(data, options);
@@ -159,8 +127,11 @@ export class Project {
   // uh maybe
   // maybe this second arg is config - like env, branch rules, serialisation rules
   // stuff that's external to the actual project and managed by the repo
+
+  // TODO maybe the constructor is (data, Workspace)
   constructor(data: l.Project, repoConfig: RepoOptions = {}) {
-    this.repo = setConfigDefaults(repoConfig);
+    this.setConfig(repoConfig);
+
     this.name = data.name;
     this.description = data.description;
     this.openfn = data.openfn;
@@ -171,6 +142,10 @@ export class Project {
     this.meta = data.meta;
   }
 
+  setConfig(config: Partial<WorkspaceConfig>) {
+    this.config = buildConfig(config);
+  }
+
   serialize(type: 'json' | 'yaml' | 'fs' | 'state' = 'json', options?: any) {
     if (type in serializers) {
       // @ts-ignore