diff --git a/.changeset/tired-donkeys-leave.md b/.changeset/tired-donkeys-leave.md new file mode 100644 index 000000000..d7cbf6c6a --- /dev/null +++ b/.changeset/tired-donkeys-leave.md @@ -0,0 +1,6 @@ +--- +'@openfn/project': minor +'@openfn/cli': minor +--- + +Adds new form of json diffing for cli diff --git a/packages/cli/package.json b/packages/cli/package.json index bb305b4dc..65e322015 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -35,6 +35,7 @@ "devDependencies": { "@openfn/language-collections": "^0.8.3", "@openfn/language-common": "3.2.3", + "@types/json-diff": "^1.0.3", "@types/lodash-es": "~4.17.12", "@types/mock-fs": "^4.13.4", "@types/node": "^18.19.130", @@ -62,6 +63,7 @@ "dotenv": "^17.3.1", "dotenv-expand": "^12.0.3", "figures": "^5.0.0", + "json-diff": "^1.0.6", "rimraf": "^6.1.3", "treeify": "^1.1.0", "undici": "6.24.1", diff --git a/packages/cli/src/projects/deploy.ts b/packages/cli/src/projects/deploy.ts index c355f2773..8f204411b 100644 --- a/packages/cli/src/projects/deploy.ts +++ b/packages/cli/src/projects/deploy.ts @@ -41,6 +41,7 @@ export type DeployOptions = Pick< new?: boolean; name?: string; alias?: string; + jsonDiff?: boolean; }; const options = [ @@ -51,6 +52,7 @@ const options = [ o2.new, o2.name, o2.alias, + o2.jsonDiff, // general options o.apiKey, @@ -121,6 +123,12 @@ export const hasRemoteDiverged = ( return diverged; }; +export type SyncResult = { + merged: Project; + remoteProject: Project; + locallyChangedWorkflows: string[]; +}; + // This function is responsible for syncing changes in the user's local project // with the remote app version // It returns a merged state object @@ -131,7 +139,7 @@ const syncProjects = async ( localProject: Project, trackedProject: Project, // the project we want to update logger: Logger -): Promise => { +): Promise => { // First step, fetch the latest version and write // this may throw! let remoteProject: Project; @@ -169,15 +177,10 @@ const syncProjects = async ( ); // TODO: what if remote diff and the version checked disagree for some reason? - let diffs = []; - if (locallyChangedWorkflows.length) { - diffs = reportDiff( - localProject, - remoteProject, - locallyChangedWorkflows, - logger - ); - } + const diffs = locallyChangedWorkflows.length + ? remoteProject.diff(localProject, locallyChangedWorkflows) + : []; + if (!diffs.length) { logger.success('Nothing to deploy'); return null; @@ -238,7 +241,7 @@ const syncProjects = async ( onlyUpdated: true, }); - return merged; + return { merged, remoteProject, locallyChangedWorkflows }; }; export async function handler(options: DeployOptions, logger: Logger) { @@ -306,11 +309,14 @@ export async function handler(options: DeployOptions, logger: Logger) { `Loaded checked-out project ${printProjectName(localProject)}` ); - let merged; + let merged: Project; + let remoteProject: Project | undefined; + let locallyChangedWorkflows: string[] = []; + if (options.new) { merged = localProject; } else { - merged = await syncProjects( + const syncResult = await syncProjects( options, config, ws, @@ -318,9 +324,10 @@ export async function handler(options: DeployOptions, logger: Logger) { tracker, logger ); - if (!merged) { + if (!syncResult) { return; } + ({ merged, remoteProject, locallyChangedWorkflows } = syncResult); } const state = merged.serialize('state', { @@ -335,7 +342,21 @@ export async function handler(options: DeployOptions, logger: Logger) { logger.debug(JSON.stringify(state, null, 2)); logger.debug(); - // TODO: I want to report diff HERE, after the merged state and stuff has been built + if (remoteProject) { + if (options.jsonDiff) { + const remoteState = remoteProject.serialize('state', { + format: 'json', + }) as object; + await printJsonDiff(remoteState, state as object, logger); + } else { + printRichDiff( + localProject, + remoteProject, + locallyChangedWorkflows, + logger + ); + } + } if (options.dryRun) { logger.always('dryRun option set: skipping upload step'); @@ -404,7 +425,65 @@ export async function handler(options: DeployOptions, logger: Logger) { } } -export const reportDiff = ( +const printStepChanges = ( + localWf: ReturnType, + remoteWf: ReturnType, + logger: Logger +) => { + if (!localWf || !remoteWf) return; + + const localSteps = localWf.steps as any[]; + const remoteSteps = remoteWf.steps as any[]; + const remoteById = Object.fromEntries(remoteSteps.map((s) => [s.id, s])); + const localById = Object.fromEntries(localSteps.map((s) => [s.id, s])); + + for (const step of localSteps) { + const remote = remoteById[step.id]; + if (!remote) { + logger.always(c.yellow(` ${step.id || step.name}: added`)); + continue; + } + + const lines: string[] = []; + + if (step.name !== remote.name) { + lines.push(` - name: "${remote.name}" -> "${step.name}"`); + } + + const localAdaptor = step.adaptor ?? step.adaptors?.[0]; + const remoteAdaptor = remote.adaptor ?? remote.adaptors?.[0]; + if (localAdaptor !== remoteAdaptor) { + lines.push(` - adaptor: ${remoteAdaptor} -> ${localAdaptor}`); + } + + const localExpr = step.expression ?? step.body ?? ''; + const remoteExpr = remote.expression ?? remote.body ?? ''; + if (localExpr !== remoteExpr) { + const localLines = localExpr.split('\n').length; + const remoteLines = remoteExpr.split('\n').length; + const added = Math.max(0, localLines - remoteLines); + const removed = Math.max(0, remoteLines - localLines); + const summary = + added === 0 && removed === 0 ? '' : `<+${added}/-${removed}>`; + lines.push(` - body: ${summary}`); + } + + if (lines.length) { + logger.always(c.yellow(` ${step.id || step.name}:`)); + for (const line of lines) { + logger.always(c.yellow(line)); + } + } + } + + for (const step of remoteSteps) { + if (!localById[step.id]) { + logger.always(c.yellow(` ${step.id || step.name}: removed`)); + } + } +}; + +export const printRichDiff = ( local: Project, remote: Project, locallyChangedWorkflows: string[], @@ -416,36 +495,53 @@ export const reportDiff = ( return diffs; } - const added = diffs.filter((d) => d.type === 'added'); - const changed = diffs.filter((d) => d.type === 'changed'); const removed = diffs.filter((d) => d.type === 'removed'); + const changed = diffs.filter((d) => d.type === 'changed'); + const added = diffs.filter((d) => d.type === 'added'); - if (added.length > 0) { + if (removed.length > 0) { logger.break(); - logger.always(c.green('Workflows added:')); - for (const diff of added) { - logger.always(c.green(` - ${diff.id}`)); + for (const diff of removed) { + const wf = remote.getWorkflow(diff.id); + const label = wf?.name || diff.id; + logger.always(c.red(`${label}: deleted`)); } - logger.break(); } if (changed.length > 0) { logger.break(); - logger.always(c.yellow('Workflows modified:')); for (const diff of changed) { - logger.always(c.yellow(` - ${diff.id}`)); + const localWf = local.getWorkflow(diff.id); + const remoteWf = remote.getWorkflow(diff.id); + const label = localWf?.name || diff.id; + logger.always(c.yellow(`${label}: changed`)); + printStepChanges(localWf, remoteWf, logger); } - logger.break(); } - if (removed.length > 0) { + if (added.length > 0) { logger.break(); - logger.always(c.red('Workflows removed:')); - for (const diff of removed) { - logger.always(c.red(` - ${diff.id}`)); + for (const diff of added) { + const wf = local.getWorkflow(diff.id); + const label = wf?.name || diff.id; + logger.always(c.green(`${label}: added`)); } - logger.break(); } + logger.break(); return diffs; }; + +export const printJsonDiff = async ( + remoteState: object, + mergedState: object, + logger: Logger +) => { + const { default: jsondiff } = await import('json-diff'); + const diff = jsondiff.diffString(remoteState, mergedState); + if (diff) { + logger.break(); + logger.always(diff); + logger.break(); + } +}; diff --git a/packages/cli/src/projects/options.ts b/packages/cli/src/projects/options.ts index 7da60ea24..5c49bac3e 100644 --- a/packages/cli/src/projects/options.ts +++ b/packages/cli/src/projects/options.ts @@ -119,4 +119,13 @@ export const name: CLIOption = { }, }; +export const jsonDiff: CLIOption = { + name: 'json-diff', + yargs: { + boolean: true, + description: + 'Show a full JSON diff of the project state instead of the default rich text summary', + }, +}; + export { newProject as new }; diff --git a/packages/cli/test/projects/deploy.test.ts b/packages/cli/test/projects/deploy.test.ts index 183b38678..d8832c290 100644 --- a/packages/cli/test/projects/deploy.test.ts +++ b/packages/cli/test/projects/deploy.test.ts @@ -9,7 +9,7 @@ import createLightningServer from '@openfn/lightning-mock'; import { handler as deploy, hasRemoteDiverged, - reportDiff, + printRichDiff, } from '../../src/projects/deploy'; import { myProject_yaml, myProject_v1, UUID } from './fixtures'; import { checkout } from '../../src/projects'; @@ -194,7 +194,7 @@ test.serial( } ); -test('reportDiff: should report no changes for identical projects', (t) => { +test('printRichDiff: should report no changes for identical projects', (t) => { const wf = generateWorkflow('@id a trigger-x'); const local = new Project({ @@ -207,7 +207,7 @@ test('reportDiff: should report no changes for identical projects', (t) => { workflows: [wf], }); - const diffs = reportDiff(local, remote, [], logger); + const diffs = printRichDiff(local, remote, [], logger); t.is(diffs.length, 0); const { message, level } = logger._parse(logger._last); @@ -215,7 +215,7 @@ test('reportDiff: should report no changes for identical projects', (t) => { t.is(message, 'No workflow changes detected'); }); -test('reportDiff: should report changed workflow', (t) => { +test('printRichDiff: should report changed workflow', (t) => { const wfRemote = generateWorkflow('@id a trigger-x'); const wfLocal = generateWorkflow('@id a trigger-y'); @@ -229,15 +229,14 @@ test('reportDiff: should report changed workflow', (t) => { workflows: [wfRemote], }); - const diffs = reportDiff(local, remote, [], logger); + const diffs = printRichDiff(local, remote, [], logger); t.is(diffs.length, 1); t.deepEqual(diffs[0], { id: 'a', type: 'changed' }); - t.truthy(logger._find('always', /workflows modified/i)); - t.truthy(logger._find('always', /- a/i)); + t.truthy(logger._find('always', /: changed/i)); }); -test('reportDiff: should report added workflow', (t) => { +test('printRichDiff: should report added workflow', (t) => { const wf1 = generateWorkflow('@id a trigger-x'); const wf2 = generateWorkflow('@id b trigger-y'); @@ -251,15 +250,14 @@ test('reportDiff: should report added workflow', (t) => { workflows: [wf1], }); - const diffs = reportDiff(local, remote, [], logger); + const diffs = printRichDiff(local, remote, [], logger); t.is(diffs.length, 1); t.deepEqual(diffs[0], { id: 'b', type: 'added' }); - t.truthy(logger._find('always', /workflows added/i)); - t.truthy(logger._find('always', /- b/i)); + t.truthy(logger._find('always', /: added/i)); }); -test('reportDiff: should report removed workflow', (t) => { +test('printRichDiff: should report removed workflow', (t) => { const wf1 = generateWorkflow('@id a trigger-x'); const wf2 = generateWorkflow('@id b trigger-y'); @@ -273,15 +271,14 @@ test('reportDiff: should report removed workflow', (t) => { workflows: [wf1, wf2], }); - const diffs = reportDiff(local, remote, [], logger); + const diffs = printRichDiff(local, remote, [], logger); t.is(diffs.length, 1); t.deepEqual(diffs[0], { id: 'b', type: 'removed' }); - t.truthy(logger._find('always', /workflows removed/i)); - t.truthy(logger._find('always', /- b/i)); + t.truthy(logger._find('always', /: deleted/i)); }); -test('reportDiff: should report mix of added, changed, and removed workflows', (t) => { +test('printRichDiff: should report mix of added, changed, and removed workflows', (t) => { const wf1 = generateWorkflow('@id a trigger-x'); const wf2Remote = generateWorkflow('@id b trigger-y'); const wf2Local = generateWorkflow('@id b trigger-different'); @@ -298,7 +295,7 @@ test('reportDiff: should report mix of added, changed, and removed workflows', ( workflows: [wf1, wf2Remote, wf3], // has a, b, c }); - const diffs = reportDiff(local, remote, [], logger); + const diffs = printRichDiff(local, remote, [], logger); t.is(diffs.length, 3); t.deepEqual( @@ -314,12 +311,9 @@ test('reportDiff: should report mix of added, changed, and removed workflows', ( { id: 'd', type: 'added' } ); - t.truthy(logger._find('always', /workflows added/i)); - t.truthy(logger._find('always', /- d/i)); - t.truthy(logger._find('always', /workflows modified/i)); - t.truthy(logger._find('always', /- b/i)); - t.truthy(logger._find('always', /workflows removed/i)); - t.truthy(logger._find('always', /- c/i)); + t.truthy(logger._find('always', /: added/i)); + t.truthy(logger._find('always', /: changed/i)); + t.truthy(logger._find('always', /: deleted/i)); }); test('hasRemoteDiverged: 1 workflow, no diverged', (t) => { diff --git a/packages/project/src/util/base-merge.ts b/packages/project/src/util/base-merge.ts index a27808583..32573b574 100644 --- a/packages/project/src/util/base-merge.ts +++ b/packages/project/src/util/base-merge.ts @@ -12,5 +12,5 @@ export default function baseMerge( assigns: Record, unknown> = {} ) { const pickedSource = sourceKeys ? pick(source, sourceKeys) : source; - return assign(target, { ...pickedSource, ...assigns }); + return assign({}, target, { ...pickedSource, ...assigns }); } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 033b33180..d49feb096 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -200,6 +200,9 @@ importers: figures: specifier: ^5.0.0 version: 5.0.0 + json-diff: + specifier: ^1.0.6 + version: 1.0.6 rimraf: specifier: ^6.1.3 version: 6.1.3 @@ -222,6 +225,9 @@ importers: '@openfn/language-common': specifier: 3.2.3 version: 3.2.3 + '@types/json-diff': + specifier: ^1.0.3 + version: 1.0.3 '@types/lodash-es': specifier: ~4.17.12 version: 4.17.12