diff --git a/.changeset/rename-workflow-deploy.md b/.changeset/rename-workflow-deploy.md new file mode 100644 index 000000000..ac9d7f2a3 --- /dev/null +++ b/.changeset/rename-workflow-deploy.md @@ -0,0 +1,6 @@ +--- +'@openfn/cli': patch +'@openfn/project': patch +--- + +Fix deploying an edit to a renamed workflow deleting it from Lightning diff --git a/packages/cli/src/projects/deploy.ts b/packages/cli/src/projects/deploy.ts index 80e3d3dcd..e982470e1 100644 --- a/packages/cli/src/projects/deploy.ts +++ b/packages/cli/src/projects/deploy.ts @@ -25,6 +25,7 @@ import { import { build, ensure } from '../util/command-builders'; import { printRichDiff } from './diff'; import ensureCheckout from './ensure-checkout'; +import { handler as checkout } from './checkout'; import isUuid from '../util/is-uuid'; import { getCredentialsVisitor, remapCredentials } from './credentials-helpers'; @@ -639,28 +640,6 @@ export async function handler(options: DeployOptions, logger: Logger) { if (options.checkout !== false) { const workspacePath = options.workspace ?? process.cwd(); - // Write the updated openfn.yaml and checkout file - // TODO: allow us to suppress writing this stuff - // (useful if posting from spec) - const configData = finalProject.generateConfig(); - const configPath = path.resolve(workspacePath, configData.path); - await mkdir(path.dirname(configPath), { recursive: true }); - await writeFile(configPath, configData.content); - await writeCheckoutFile(workspacePath, finalProject, options.branch); - - // Sync collections into resources.yaml (anything else in there is left alone) - const resourcesPath = path.resolve(workspacePath, RESOURCES_FILE); - const existingResources = await readFile(resourcesPath, 'utf8').catch( - (e) => { - if (e.code !== 'ENOENT') throw e; - return undefined; - } - ); - const resources = finalProject.generateResources(existingResources); - if (resources.content) { - await writeFile(resourcesPath, resources.content); - } - // TODO if this was marked as new, we probably need to ensure a unique alias here const finalOutputPath = getSerializePath( finalProject, @@ -668,6 +647,45 @@ export async function handler(options: DeployOptions, logger: Logger) { ); const fullFinalPath = await serialize(finalProject, finalOutputPath); logger.debug('Updated local project at ', fullFinalPath); + + if (filePath || options.workflow?.length) { + // workflows/ wasn't (all) deployed: rewriting it could lose local + // changes, so just write the updated openfn.yaml and checkout file + // TODO: allow us to suppress writing this stuff + // (useful if posting from spec) + const configData = finalProject.generateConfig(); + const configPath = path.resolve(workspacePath, configData.path); + await mkdir(path.dirname(configPath), { recursive: true }); + await writeFile(configPath, configData.content); + await writeCheckoutFile(workspacePath, finalProject, options.branch); + + // Sync collections into resources.yaml (anything else in there is left alone) + const resourcesPath = path.resolve(workspacePath, RESOURCES_FILE); + const existingResources = await readFile(resourcesPath, 'utf8').catch( + (e) => { + if (e.code !== 'ENOENT') throw e; + return undefined; + } + ); + const resources = finalProject.generateResources(existingResources); + if (resources.content) { + await writeFile(resourcesPath, resources.content); + } + } else { + // Check out what we just deployed, as pull does, so that workflows/ + // matches Lightning (eg, a renamed workflow moves to a folder named + // after its new id). Everything local has just been deployed, so force + await checkout( + { + project: fullFinalPath, + workspace: workspacePath, + branch: options.branch, + force: true, + track: true, + } as any, + logger + ); + } } if (options.new) { diff --git a/packages/cli/test/projects/deploy.test.ts b/packages/cli/test/projects/deploy.test.ts index 6abddc2a2..f6764be58 100644 --- a/packages/cli/test/projects/deploy.test.ts +++ b/packages/cli/test/projects/deploy.test.ts @@ -471,6 +471,78 @@ test.serial('deploy a change to a project', async (t) => { t.is(Object.keys(server.state.projects).length, 1); }); +const renameWorkflow = async (folder: string) => { + const wfPath = `/ws/workflows/${folder}/my-workflow.yaml`; + const wfYaml = fs.readFileSync(wfPath, 'utf8'); + await writeFile( + wfPath, + wfYaml.replace('name: My Workflow', 'name: My Workflow (renamed)') + ); +}; + +const deployOptions = { + endpoint: ENDPOINT, + apiKey: 'test-api-key', + workspace: '/ws', + confirm: false, +} as any; + +test.serial( + 'deploy a renamed workflow, then deploy an edit to it', + async (t) => { + await setup(projectYaml); + + await renameWorkflow('my-workflow'); + await deploy(deployOptions, logger); + + // the workflow is checked out under its new id + t.false(fs.existsSync('/ws/workflows/my-workflow')); + const wfYaml = fs.readFileSync( + '/ws/workflows/my-workflow-renamed/my-workflow-renamed.yaml', + 'utf8' + ); + t.regex(wfYaml, /^id: my-workflow-renamed$/m); + t.regex(readCheckout()!, /forked_from:\n my-workflow-renamed: /); + + await writeFile( + '/ws/workflows/my-workflow-renamed/transform-data.js', + 'log()' + ); + await deploy(deployOptions, logger); + + const workflows = Object.values( + server.state.projects[UUID].workflows + ) as any[]; + t.is(workflows.length, 1); + t.is(workflows[0].id, '72ca3eb0-042c-47a0-a2a1-a545ed4a8406'); + t.is(workflows[0].name, 'My Workflow (renamed)'); + t.is( + Object.values(workflows[0].jobs).find( + (j) => j.name === 'Transform data' + ).body, + 'log()' + ); + } +); + +test.serial( + 'deploy a renamed workflow replaces its aliased folder with one named after the new id', + async (t) => { + await setup(projectYaml); + fs.renameSync('/ws/workflows/my-workflow', '/ws/workflows/sample'); + + await renameWorkflow('sample'); + await deploy(deployOptions, logger); + + t.false(fs.existsSync('/ws/workflows/sample')); + t.true( + fs.existsSync( + '/ws/workflows/my-workflow-renamed/my-workflow-renamed.yaml' + ) + ); + } +); + test.serial( 'Error if the remote and local workflows have diverged', async (t) => { diff --git a/packages/project/src/serialize/to-app-state.ts b/packages/project/src/serialize/to-app-state.ts index 7a03372f6..88f7438fc 100644 --- a/packages/project/src/serialize/to-app-state.ts +++ b/packages/project/src/serialize/to-app-state.ts @@ -89,10 +89,26 @@ export default function ( owner: c.owner, })); + // A workflow both removed and kept (eg, its id changed after a rename) is + // a change, not a delete: never send a delete for a uuid we're also sending + const keptUuids = new Set( + project.workflows + .filter((w) => !w.isRemoved?.()) + .map((w) => w.openfn?.uuid) + .filter(Boolean) + ); state.workflows = project.workflows + .filter((w) => !(w.isRemoved?.() && keptUuids.has(w.openfn?.uuid))) .map((w) => mapWorkflow(w, project.credentials)) .reduce((obj: any, wf) => { - obj[slugify(wf.name ?? wf.id)] = wf; + const key = slugify(wf.name ?? wf.id); + // don't let one workflow (or its delete) silently overwrite another + if (key in obj) { + throw new Error( + `More than one workflow maps to "${key}" - refusing to build a state that would lose one of them` + ); + } + obj[key] = wf; return obj; }, {}); diff --git a/packages/project/test/serialize/to-app-state.test.ts b/packages/project/test/serialize/to-app-state.test.ts index 4802b904b..1c422451d 100644 --- a/packages/project/test/serialize/to-app-state.test.ts +++ b/packages/project/test/serialize/to-app-state.test.ts @@ -742,6 +742,36 @@ test('a removed workflow becomes a minimal delete: true entry', (t) => { }); }); +test('a workflow removed and kept under the same uuid is sent as a change, not a delete', (t) => { + // eg, the local id drifted after a rename: the old id is removed, the new one added + const data = cloneDeep(removableWorkflowData); + // the merge lists the kept workflow before the removed one + data.workflows.unshift({ ...cloneDeep(data.workflows[0]), id: 'wf-renamed' }); + const project = new Project(data); + project.getWorkflow('wf')!.remove(); + + const state = toAppState(project, { format: 'json' }) as Provisioner.Project; + + t.is(Object.keys(state.workflows).length, 1); + t.is(state.workflows['wf'].id, 'wf-uuid'); + t.falsy(state.workflows['wf'].delete); + t.is(state.workflows['wf'].jobs.step.id, 'step-uuid'); +}); + +test('throws if a removed workflow would overwrite another with the same name', (t) => { + const data = cloneDeep(removableWorkflowData); + const added = cloneDeep(data.workflows[0]); + added.id = 'wf-renamed'; + delete added.openfn; // never synced, so no uuid + data.workflows.push(added); + const project = new Project(data); + project.getWorkflow('wf')!.remove(); + + t.throws(() => toAppState(project, { format: 'json' }), { + message: /More than one workflow maps to "wf"/, + }); +}); + test('should serialize channels to app state', (t) => { const channels = [ {