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/integration-tests/cli/test/deploy.v2.test.ts b/integration-tests/cli/test/deploy.v2.test.ts index df844f5fd..1aaa8ceb7 100644 --- a/integration-tests/cli/test/deploy.v2.test.ts +++ b/integration-tests/cli/test/deploy.v2.test.ts @@ -205,8 +205,14 @@ test.serial('pull, change and re-deploy twice', async (t) => { ); t.falsy(stderr); const logs = extractLogs(stdout); - assertLog(t, logs, /Updated project/); - assertLog(t, logs, /Workflows modified/); + assertLog( + t, + logs, + /This will make the following changes to the remote project:/ + ); + assertLog(t, logs, /My Workflow: changed/); + assertLog(t, logs, /My Job:/gm); + assertLog(t, logs, /- body: /gm); proj = server.state.projects[projectId]; t.regex(proj.workflows['my-workflow-1'].jobs['my-job'].body, /v\: 2/); @@ -265,7 +271,9 @@ test.serial('deploy then pull, change one workflow, deploy', async (t) => { // another-workflow should appear in the modified list const anotherLog = logs.find( - (log) => log.level === 'always' && /another-workflow/.test(`${log.message}`) + (log) => + log.level === 'always' && + /Another Workflow: changed/.test(`${log.message}`) ); t.truthy(anotherLog); 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..38ac10184 100644 --- a/packages/cli/src/projects/deploy.ts +++ b/packages/cli/src/projects/deploy.ts @@ -1,6 +1,5 @@ import yargs from 'yargs'; import Project, { versionsEqual, Workspace } from '@openfn/project'; -import c from 'chalk'; import { writeFile } from 'node:fs/promises'; import path from 'node:path'; @@ -17,6 +16,7 @@ import { AuthOptions, } from './util'; import { build, ensure } from '../util/command-builders'; +import { printRichDiff } from './diff'; import type { Provisioner } from '@openfn/lexicon/lightning'; import type { Logger } from '../util/logger'; @@ -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,48 +425,16 @@ export async function handler(options: DeployOptions, logger: Logger) { } } -export const reportDiff = ( - local: Project, - remote: Project, - locallyChangedWorkflows: string[], +export const printJsonDiff = async ( + remoteState: object, + mergedState: object, logger: Logger ) => { - const diffs = remote.diff(local, locallyChangedWorkflows); - if (diffs.length === 0) { - logger.info('No workflow changes detected'); - 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'); - - if (added.length > 0) { - logger.break(); - logger.always(c.green('Workflows added:')); - for (const diff of added) { - logger.always(c.green(` - ${diff.id}`)); - } - logger.break(); - } - - if (changed.length > 0) { + const { default: jsondiff } = await import('json-diff'); + const diff = jsondiff.diffString(remoteState, mergedState); + if (diff) { logger.break(); - logger.always(c.yellow('Workflows modified:')); - for (const diff of changed) { - logger.always(c.yellow(` - ${diff.id}`)); - } + logger.always(diff); logger.break(); } - - if (removed.length > 0) { - logger.break(); - logger.always(c.red('Workflows removed:')); - for (const diff of removed) { - logger.always(c.red(` - ${diff.id}`)); - } - logger.break(); - } - - return diffs; }; diff --git a/packages/cli/src/projects/diff.ts b/packages/cli/src/projects/diff.ts new file mode 100644 index 000000000..c05bd568c --- /dev/null +++ b/packages/cli/src/projects/diff.ts @@ -0,0 +1,110 @@ +import c from 'chalk'; +import Project, { generateStepDiff, generateEdgeDiff } from '@openfn/project'; +import type { StepChange, EdgeChange } from '@openfn/project'; +import type { Logger } from '../util/logger'; + +export { generateStepDiff, generateEdgeDiff }; +export type { StepChange, EdgeChange }; + +const printEdgeDiff = (edges: EdgeChange[], logger: Logger) => { + for (const edge of edges) { + if (edge.type === 'added') { + logger.always(c.green(` ${edge.id}: added`)); + } else if (edge.type === 'removed') { + logger.always(c.red(` ${edge.id}: removed`)); + } else if (edge.type === 'changed' && edge.changes) { + logger.always(c.yellow(` ${edge.id}:`)); + const { condition, label, enabled } = edge.changes; + if (condition) + logger.always( + c.yellow( + ` - condition: ${condition.from ?? 'none'} -> ${ + condition.to ?? 'none' + }` + ) + ); + if (label) + logger.always( + c.yellow( + ` - label: "${label.from ?? ''}" -> "${label.to ?? ''}"` + ) + ); + if (enabled) + logger.always( + c.yellow(` - enabled: ${enabled.from} -> ${enabled.to}`) + ); + } + } +}; + +const printStepDiff = (steps: StepChange[], logger: Logger) => { + for (const step of steps) { + if (step.type === 'added') { + logger.always(c.green(` ${step.name}: added`)); + } else if (step.type === 'removed') { + logger.always(c.red(` ${step.name}: removed`)); + } else if (step.type === 'changed' && step.changes) { + logger.always(c.yellow(` ${step.name}:`)); + const { name, adaptor, body } = step.changes; + if (name) + logger.always(c.yellow(` - name: "${name.from}" -> "${name.to}"`)); + if (adaptor) + logger.always( + c.yellow(` - adaptor: ${adaptor.from} -> ${adaptor.to}`) + ); + if (body) logger.always(c.yellow(` - body: ${body}`)); + } + } +}; + +export const printRichDiff = ( + local: Project, + remote: Project, + locallyChangedWorkflows: string[], + logger: Logger +) => { + const diffs = remote.diff(local, locallyChangedWorkflows); + if (diffs.length === 0) { + logger.info('No workflow changes detected'); + return diffs; + } + + const removed = diffs.filter((d) => d.type === 'removed'); + const changed = diffs.filter((d) => d.type === 'changed'); + const added = diffs.filter((d) => d.type === 'added'); + + logger.always('This will make the following changes to the remote project:'); + + if (removed.length > 0) { + logger.break(); + for (const diff of removed) { + const wf = remote.getWorkflow(diff.id); + const label = wf?.name || diff.id; + logger.always(c.red(`${label}: deleted`)); + } + } + + if (changed.length > 0) { + logger.break(); + for (const diff of changed) { + const localWf = local.getWorkflow(diff.id); + const remoteWf = remote.getWorkflow(diff.id); + const label = localWf?.name || diff.id; + logger.always(c.yellow(`${label}: changed`)); + printStepDiff(generateStepDiff(localWf, remoteWf), logger); + printEdgeDiff(generateEdgeDiff(localWf, remoteWf), logger); + } + } + + if (added.length > 0) { + logger.break(); + 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(); + return diffs; +}; 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..2be2f28c2 100644 --- a/packages/cli/test/projects/deploy.test.ts +++ b/packages/cli/test/projects/deploy.test.ts @@ -9,8 +9,8 @@ import createLightningServer from '@openfn/lightning-mock'; import { handler as deploy, hasRemoteDiverged, - reportDiff, } from '../../src/projects/deploy'; +import { printRichDiff } from '../../src/projects/diff'; 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/index.ts b/packages/project/src/index.ts index 89b46b8da..431036dd7 100644 --- a/packages/project/src/index.ts +++ b/packages/project/src/index.ts @@ -10,6 +10,13 @@ export { generateWorkflow, generateProject } from './gen/generator'; export { diff } from './util/project-diff'; export type { WorkflowDiff, DiffType } from './util/project-diff'; + +export { generateStepDiff, generateEdgeDiff } from './util/workflow-diff'; +export type { + StepChange, + StepChangeType, + EdgeChange, +} from './util/workflow-diff'; export { generateHash as generateVersionHash, match as versionsEqual, 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/packages/project/src/util/version.ts b/packages/project/src/util/version.ts index df42b8bb1..eb0cc57ed 100644 --- a/packages/project/src/util/version.ts +++ b/packages/project/src/util/version.ts @@ -115,9 +115,18 @@ export const generateHash = ( const edges = Object.values(wfState.edges) .map((edge) => { - const source = uuidMap[edge.source_trigger_id! ?? edge.source_job_id]; + const sourceId = edge.source_trigger_id! ?? edge.source_job_id; + const source = uuidMap[sourceId]; const target = uuidMap[edge.target_job_id]; + if (!source || !target) { + throw new Error( + `Edge references unknown ${ + !source ? `source "${sourceId}"` : `target "${edge.target_job_id}"` + }. seems a node's id changed without edges connecting to it being updated.` + ); + } + (edge as any).name = `${source.name ?? source.id}-${ target.name ?? target.id }`; diff --git a/packages/project/src/util/workflow-diff.ts b/packages/project/src/util/workflow-diff.ts new file mode 100644 index 000000000..464b61a6c --- /dev/null +++ b/packages/project/src/util/workflow-diff.ts @@ -0,0 +1,151 @@ +import Workflow from '../Workflow'; + +export type StepChangeType = 'added' | 'removed' | 'changed'; + +export type StepChange = { + id: string; + name: string; + type: StepChangeType; + changes?: { + name?: { from: string; to: string }; + adaptor?: { from: string; to: string }; + body?: string; + }; +}; + +const TRACKED_FIELDS: Array<{ key: 'name' | 'adaptor' }> = [ + { key: 'name' }, + { key: 'adaptor' }, +]; + +export const generateStepDiff = ( + localWf: Workflow | undefined, + remoteWf: Workflow | undefined +): StepChange[] => { + 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])); + + const changes: StepChange[] = []; + + for (const step of localSteps) { + const remote = remoteById[step.id]; + if (!remote) { + changes.push({ id: step.id, name: step.name || step.id, type: 'added' }); + continue; + } + + const fieldChanges: StepChange['changes'] = {}; + + for (const { key } of TRACKED_FIELDS) { + const localVal = step[key]; + const remoteVal = remote[key]; + if (localVal !== remoteVal) { + fieldChanges[key] = { from: remoteVal, to: localVal }; + } + } + + const localExpr = step.expression ?? step.body ?? ''; + const remoteExpr = remote.expression ?? remote.body ?? ''; + if (localExpr !== remoteExpr) { + const lineDiff = + localExpr.split('\n').length - remoteExpr.split('\n').length; + fieldChanges.body = + lineDiff > 0 + ? `+${lineDiff} lines` + : lineDiff < 0 + ? `${lineDiff} lines` + : ''; + } + + if (Object.keys(fieldChanges).length > 0) { + changes.push({ + id: step.id, + name: step.name || step.id, + type: 'changed', + changes: fieldChanges, + }); + } + } + + for (const step of remoteSteps) { + if (!localById[step.id]) { + changes.push({ + id: step.id, + name: step.name || step.id, + type: 'removed', + }); + } + } + + return changes; +}; + +export type EdgeChange = { + id: string; + type: StepChangeType; + changes?: { + condition?: { from: string | undefined; to: string | undefined }; + label?: { from: string | undefined; to: string | undefined }; + enabled?: { from: boolean; to: boolean }; + }; +}; + +const getEdgeMap = (wf: Workflow): Record => { + const map: Record = {}; + for (const step of wf.steps as any[]) { + const next = + typeof step.next === 'string' ? { [step.next]: {} } : step.next ?? {}; + for (const [targetId, rules] of Object.entries(next)) { + map[`${step.id}->${targetId}`] = + typeof rules === 'object' && rules !== null ? rules : {}; + } + } + return map; +}; + +export const generateEdgeDiff = ( + localWf: Workflow | undefined, + remoteWf: Workflow | undefined +): EdgeChange[] => { + if (!localWf || !remoteWf) return []; + + const localEdges = getEdgeMap(localWf); + const remoteEdges = getEdgeMap(remoteWf); + const changes: EdgeChange[] = []; + + for (const [id, local] of Object.entries(localEdges)) { + const remote = remoteEdges[id]; + if (!remote) { + changes.push({ id, type: 'added' }); + continue; + } + + const fieldChanges: EdgeChange['changes'] = {}; + + if (local.condition !== remote.condition) { + fieldChanges.condition = { from: remote.condition, to: local.condition }; + } + if (local.label !== remote.label) { + fieldChanges.label = { from: remote.label, to: local.label }; + } + if (!!local.disabled !== !!remote.disabled) { + fieldChanges.enabled = { from: !remote.disabled, to: !local.disabled }; + } + + if (Object.keys(fieldChanges).length > 0) { + changes.push({ id, type: 'changed', changes: fieldChanges }); + } + } + + for (const id of Object.keys(remoteEdges)) { + if (!localEdges[id]) { + changes.push({ id, type: 'removed' }); + } + } + + return changes; +}; 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