diff --git a/.changeset/fast-boats-grin.md b/.changeset/fast-boats-grin.md new file mode 100644 index 000000000..1495c1e85 --- /dev/null +++ b/.changeset/fast-boats-grin.md @@ -0,0 +1,5 @@ +--- +'@openfn/project': patch +--- + +Fix an issue where serializing a project without credential uuids to app state causes credentials on job bodies to be lost diff --git a/.changeset/fast-cobras-throw.md b/.changeset/fast-cobras-throw.md new file mode 100644 index 000000000..4d2c25d31 --- /dev/null +++ b/.changeset/fast-cobras-throw.md @@ -0,0 +1,5 @@ +--- +'@openfn/lexicon': patch +--- + +Project credentials can optionally have a UUID diff --git a/.changeset/young-brooms-drum.md b/.changeset/young-brooms-drum.md new file mode 100644 index 000000000..aa820287d --- /dev/null +++ b/.changeset/young-brooms-drum.md @@ -0,0 +1,5 @@ +--- +'@openfn/cli': patch +--- + +Enable a v2 project yaml file to be deployed directly, eg, `openfn project deploy .projects/main@app.openfn.org.yaml diff --git a/packages/cli/src/projects/deploy.ts b/packages/cli/src/projects/deploy.ts index 65f64d6ab..bfdeb6f8a 100644 --- a/packages/cli/src/projects/deploy.ts +++ b/packages/cli/src/projects/deploy.ts @@ -76,7 +76,7 @@ const printProjectName = (project: Project) => export const command: yargs.CommandModule = { command: 'deploy [project]', aliases: 'push', - describe: `Deploy the checked out project to a Lightning Instance`, + describe: `Deploy the passed or checked-out project to a Lightning Instance`, builder: (yargs: yargs.Argv) => build(options, yargs) .positional('project', { @@ -275,25 +275,53 @@ export async function handler(options: DeployOptions, logger: Logger) { ); const config = loadAppAuthConfig(options, logger); - // TODO this is the hard way to load the local alias - // We need track alias in openfn.yaml to make this easier (and tracked in from fs) - const ws = new Workspace(options.workspace || '.'); + // The local project that we want to actually deploy + let localProject: Project; + let ws; + let alias = options.alias ?? null; - const active = ws.getTrackedProject(); - const alias = options.alias ?? active?.alias; + if (options.project) { + const localPath = path.resolve( + options.workspace ?? process.cwd(), + options.project + ); + logger.debug('Reading project from path ', localPath); + localProject = await Project.from('path', localPath); + + // If the local project doesn't have stateful stuff, + // flag this as a new upload + if (!localProject.uuid) { + logger.debug( + 'Local project does not have a UUID: assuming this is a new project deployment' + ); + options.new = true; + + // TODO ensure the alias is unique if we're posting a new project + } + } else { + logger.debug('Reading checked-out project from workspace'); + // TODO this is the hard way to load the local alias + // We need track alias in openfn.yaml to make this easier (and tracked in from fs) + ws = new Workspace(options.workspace || '.'); - const localProject = await Project.from('fs', { - root: options.workspace || '.', - alias, - name: options.name, - }); + const active = ws.getTrackedProject(); + + // messy + alias ??= active?.alias ?? null; + + localProject = await Project.from('fs', { + root: options.workspace || '.', + alias, + name: options.name, + }); + } // Track the remote we want to target // If the user passed a project alias, we need to use that // Otherwise just sync with the local project let tracker; if (!options.new) { - tracker = ws.get(options.project ?? localProject.uuid!); + tracker = ws?.get(options.project ?? localProject.uuid!); if (!tracker) { // Is this really an error? Unlikely to happen I thuink console.log( @@ -344,7 +372,7 @@ export async function handler(options: DeployOptions, logger: Logger) { const syncResult = await syncProjects( options, config, - ws, + ws!, localProject, tracker!, logger @@ -355,6 +383,10 @@ export async function handler(options: DeployOptions, logger: Logger) { ({ merged, remoteProject, locallyChangedWorkflows } = syncResult); } + /** + * Questions: + * 1. When this serializses, should project_credentials have uuids? + */ const state = merged.serialize('state', { format: 'json', }) as Provisioner.Project_v1; @@ -433,11 +465,16 @@ export async function handler(options: DeployOptions, logger: Logger) { updateForkedFrom(finalProject); const configData = finalProject.generateConfig(); + + // Write the updated openfn.yaml + // TODO: allow us to suppress writing this stuff + // (useful if posting from spec) await writeFile( - path.resolve(options.workspace!, configData.path), + path.resolve(options.workspace ?? process.cwd(), configData.path), configData.content ); + // TODO if this was marked as new, we probably need to ensure a unique alias here const finalOutputPath = getSerializePath(localProject, options.workspace!); const fullFinalPath = await serialize(finalProject, finalOutputPath); logger.debug('Updated local project at ', fullFinalPath); diff --git a/packages/cli/src/projects/util.ts b/packages/cli/src/projects/util.ts index 548cee720..64d5c7714 100644 --- a/packages/cli/src/projects/util.ts +++ b/packages/cli/src/projects/util.ts @@ -184,7 +184,6 @@ export async function deployProject( // TODO html errors are too long to be useful... figure this out later logger?.error(content); } - throw new CLIError( `Failed to deploy project ${state.name}: ${response.status}` ); diff --git a/packages/cli/test/projects/create-credentials.test.ts b/packages/cli/test/projects/create-credentials.test.ts index f1d1c6277..321931914 100644 --- a/packages/cli/test/projects/create-credentials.test.ts +++ b/packages/cli/test/projects/create-credentials.test.ts @@ -51,7 +51,7 @@ test('sync-credentials: ignores duplicate references', (t) => { t.deepEqual(findCredentialIds(project), ['same']); }); -test.only('sync-credentials: creates credential yaml file', (t) => { +test('sync-credentials: creates credential yaml file', (t) => { mock({ '/ws': {} }); const project = new Project( diff --git a/packages/cli/test/projects/deploy.test.ts b/packages/cli/test/projects/deploy.test.ts index 9b5a5a9f9..a035c9fcc 100644 --- a/packages/cli/test/projects/deploy.test.ts +++ b/packages/cli/test/projects/deploy.test.ts @@ -18,6 +18,7 @@ import { UUID, two_workflows_yaml as twowfs, TWO_WORKFLOWS_UUID, + myProject_spec, } from './fixtures'; import { checkout } from '../../src/projects'; @@ -77,18 +78,47 @@ test.beforeEach(() => { mock.restore(); }); -test.serial('deploy a new project', async (t) => { +test.serial( + 'deploy a project as new from the checked out project', + async (t) => { + // the server should have 1 registered project by default - that's fine + t.is(Object.keys(server.state.projects).length, 1); + + await setup(); + + await deploy( + { + endpoint: ENDPOINT, + apiKey: 'test-api-key', + workspace: '/ws', + new: true, + } as any, + logger + ); + + // We should now have a new project with a new UUID + t.is(Object.keys(server.state.projects).length, 2); + + const success = logger._find('success', /Created new project at/); + t.truthy(success); + } +); + +test.serial('deploy a project as new from a v2 spec yaml', async (t) => { // the server should have 1 registered project by default - that's fine t.is(Object.keys(server.state.projects).length, 1); - await setup(); + // skip the usual setup and just set up the filesystem + mockFs({ + '/ws/.projects/main@localhost.yaml': myProject_spec, + '/ws/openfn.yaml': '', // TODO this shouldn't be needed + }); await deploy( { endpoint: ENDPOINT, apiKey: 'test-api-key', - workspace: '/ws', - new: true, + project: '/ws/.projects/main@localhost.yaml', } as any, logger ); @@ -96,6 +126,37 @@ test.serial('deploy a new project', async (t) => { // We should now have a new project with a new UUID t.is(Object.keys(server.state.projects).length, 2); + const newUuid = Object.keys(server.state.projects).find((id) => id !== UUID); + const newProject = server.state.projects[newUuid!]; + + // credential should have been created with a generated uuid + t.is(newProject.project_credentials.length, 1); + const credential = newProject.project_credentials[0]; + t.is(credential.name, 'http1'); + t.is(credential.owner, 'super@openfn.org'); + t.truthy(credential.id); + + // only one workflow, keyed by a generated uuid rather than 'my-workflow' + const workflows = Object.values(newProject.workflows) as any[]; + t.is(workflows.length, 1); + const workflow = workflows[0]; + t.is(workflow.name, 'My Workflow'); + + const job = workflow.jobs['transform-data']; + t.is(job.body, 'fn()'); + t.is(job.adaptor, '@openfn/language-common@latest'); + // the job's configuration should resolve to the new credential + t.is(job.project_credential_id, credential.id); + + const trigger = workflow.triggers['webhook']; + t.truthy(trigger); + t.is(trigger.type, 'webhook'); + + const edge = workflow.edges['webhook->transform-data']; + t.truthy(edge); + t.is(edge.source_trigger_id, trigger.id); + t.is(edge.target_job_id, job.id); + const success = logger._find('success', /Created new project at/); t.truthy(success); }); diff --git a/packages/cli/test/projects/fixtures.ts b/packages/cli/test/projects/fixtures.ts index 64f7e350f..de6fc019f 100644 --- a/packages/cli/test/projects/fixtures.ts +++ b/packages/cli/test/projects/fixtures.ts @@ -104,6 +104,32 @@ workflows: id: my-workflow start: webhook`; +export const myProject_spec = `id: my-project +name: My Project +schema_version: '4.0' +description: my lovely project +collections: [] +credentials: + - name: http1 + owner: super@openfn.org +workflows: + - name: My Workflow + steps: + - id: transform-data + name: Transform data + expression: fn() + adaptor: '@openfn/language-common@latest' + configuration: super@openfn.org|http1 + - id: webhook + type: webhook + enabled: true + next: + transform-data: + disabled: false + condition: always + id: my-workflow + start: webhook`; + export const TWO_WORKFLOWS_UUID = '4b09ddf1-35f4-4e40-9aa9-0d80c086dd9e'; export const two_workflows_yaml = `id: my-project diff --git a/packages/lexicon/portability.d.ts b/packages/lexicon/portability.d.ts index c6b3cb9ed..4038f5b55 100644 --- a/packages/lexicon/portability.d.ts +++ b/packages/lexicon/portability.d.ts @@ -95,6 +95,7 @@ export interface Trigger extends Step { export interface Credential { name: string; owner: string; + uuid?: string | number; } export interface Job extends Step { diff --git a/packages/lightning-mock/test/rest.test.ts b/packages/lightning-mock/test/rest.test.ts index 21d13145e..614b3bc4b 100644 --- a/packages/lightning-mock/test/rest.test.ts +++ b/packages/lightning-mock/test/rest.test.ts @@ -127,7 +127,43 @@ test('validateProvisionPayload: returns null for a valid edge with source_job_id t.is(validateProvisionPayload(payload), null); }); -test('validateProvisionPayload: returns errors when edge has no source', (t) => { +test('validateProvisionPayload: returns errors when edge has no source job or trigger id', (t) => { + const payload = { + id: 'proj-1', + workflows: [ + { + name: 'wf1', + edges: [ + { + id: 'edge-1', + source_trigger_id: null, + target_job_id: '', + enabled: true, + }, + ], + }, + ], + }; + const result = validateProvisionPayload(payload); + t.truthy(result); + t.deepEqual(result, { + errors: { + workflows: { + wf1: { + edges: { + 'edge-1': { + source_job_id: [ + 'source_job_id or source_trigger_id must be present', + ], + }, + }, + }, + }, + }, + }); +}); + +test('validateProvisionPayload: for new edges allow source_job', (t) => { const payload = { id: 'proj-1', workflows: [ diff --git a/packages/project/src/serialize/to-app-state.ts b/packages/project/src/serialize/to-app-state.ts index 4e923401a..a20cca9dd 100644 --- a/packages/project/src/serialize/to-app-state.ts +++ b/packages/project/src/serialize/to-app-state.ts @@ -49,6 +49,13 @@ export default function ( state.id = (uuid as string) ?? randomUUID(); + // Ensure each credential has a UUID + project.credentials = + project.credentials?.map((c) => ({ + ...c, + uuid: c.uuid ?? randomUUID(), + })) ?? []; + Object.assign(state, rest, project.options); if (options.asSpec) { for (const c of project.credentials) { @@ -62,14 +69,10 @@ export default function ( }; } } else { - const credentialsWithUuids = - project.credentials?.map((c) => ({ - ...c, - uuid: (c as CredentialState).uuid ?? randomUUID(), - })) ?? []; - - state.project_credentials = credentialsWithUuids.map((c) => ({ - // note the subtle conversion here + state.project_credentials = project.credentials.map((c) => ({ + // note the subtle conversion here: uuid -> id + // That's because the local Project uses the uuid key to track UUIDs + // but the provisioner spec uses id id: c.uuid as string, name: c.name, owner: c.owner, @@ -163,23 +166,22 @@ export const mapWorkflow = ( } if ( typeof s.configuration === 'string' && + s.configuration.length && !s.configuration.endsWith('.json') ) { let projectCredentialId = s.configuration; - if (projectCredentialId) { - const mappedCredential = credentials.find((c) => { - const name = getCredentialName(c); - return name === projectCredentialId; - }); - if (mappedCredential && useUuids) { - projectCredentialId = mappedCredential.uuid; - } + const mappedCredential = credentials.find((c) => { + const name = getCredentialName(c); + return name === projectCredentialId; + }); + if (mappedCredential && useUuids) { + projectCredentialId = mappedCredential.uuid; + } - if (useUuids) { - otherOpenFnProps.project_credential_id = projectCredentialId; - } else { - otherOpenFnProps.credential = projectCredentialId; - } + if (useUuids) { + otherOpenFnProps.project_credential_id = projectCredentialId; + } else { + otherOpenFnProps.credential = projectCredentialId; } } diff --git a/packages/project/test/serialize/to-app-state.test.ts b/packages/project/test/serialize/to-app-state.test.ts index efa7990e2..9fe91cd21 100644 --- a/packages/project/test/serialize/to-app-state.test.ts +++ b/packages/project/test/serialize/to-app-state.test.ts @@ -237,7 +237,7 @@ test('should write openfn keys to objects', (t) => { t.is(state.workflows['wf'].edges['trigger->step'].x, 1); }); -test('should handle credentials', (t) => { +test('should handle credentials with existing UUIDs', (t) => { const data = { id: 'my-project', credentials: [ @@ -280,6 +280,51 @@ test('should handle credentials', (t) => { t.is(step.project_credential_id, '123'); }); +test('should handle credentials without UUIDs (ie new credentials)', (t) => { + const data = { + id: 'my-project', + credentials: [ + { + name: 'cred', + owner: 'admin@openfn.org', + }, + ], + workflows: [ + { + id: 'wf', + name: 'wf', + steps: [ + { + id: 'trigger', + type: 'webhook', + next: { + step: {}, + }, + }, + { + id: 'step', + expression: '.', + configuration: 'admin@openfn.org|cred', + openfn: { + keychain_credential_id: 'k', + }, + }, + ], + }, + ], + }; + + const state = toAppState(new Project(data), { + format: 'json', + }) as Provisioner.Project_v1; + const { step } = state.workflows['wf'].jobs; + t.is(step.keychain_credential_id, 'k'); + + // Should look like a UUID + t.is(typeof step.project_credential_id, 'string'); + t.is(step.project_credential_id!.length, 36); +}); + test('should force a UUID on project credentials', (t) => { const data = { id: 'my-project',