Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: log workflowGroup from the exit-handler TDE-1230 #703

Closed
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
"build": "tsc",
"lint": "npx eslint . --ignore-path .gitignore",
"format": "npx prettier . -w",
"test": "node --import tsx --test infra/**/*.test.ts"
"test": "node --import tsx --test infra/**/*.test.ts templates/common/__test__/*.test.mjs"
},
"devDependencies": {
"@aws-cdk/lambda-layer-kubectl-v28": "^2.1.0",
Expand Down
1 change: 1 addition & 0 deletions templates/common/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ The script ran by this template is generating a log, including the status of the
### Template usage

The information to pass to this `WorkflowTemplate` is the status and the parameters of the workflow (`workflow.status` & `workflow.parameters`).
If a workflow parameter contains a `source` parameter, it will be parsed to determine a workflow group (such as `land` or `sea`).

As the `onExit` event [does not handle a `templateRef`](https://github.com/argoproj/argo-workflows/issues/3188),
an additional template called by the `onExit` event has to be added to the templates so it can finally call the `tpl-exit-handler` template.
Expand Down
112 changes: 112 additions & 0 deletions templates/common/__test__/exit.handler.test.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
import assert from 'node:assert';
import fs from 'node:fs';
import { describe, it } from 'node:test';

/**
* Read the workflow YAML file and create a function from the script inside.
* replacing {{ inputs.* }} with ctx
*
* @param {*} ctx
* @returns Function that requires a `context` and `core` parameter
*/
function createTestFunction(ctx) {
const func = fs.readFileSync('./templates/common/exit.handler.yml', 'utf-8').split('source: |')[1];

const newFunc = func
// Replace inputs
.replace('{{= inputs.parameters.workflow_parameters }}', `${ctx.workflowParameters ?? '[]'}`)
.replace('{{inputs.parameters.workflow_status}}', `${ctx.workflowStatus ?? 'Failed'}`)
.split('\n')
.join('\n');
//console.log(newFunc);
return new Function('context', newFunc);
}

function runScript(ctx) {
const action = createTestFunction(ctx);

const output = {};
action();

return output;
}

describe('exit handler script template', () => {
it('should log workflow status and parameters', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be good to use the built in test stubber her rather than overwriting the logger

const originalLog = console.log;
let logOutput = [];
console.log = (...args) => logOutput.push(args.join(' '));

runScript({
workflowParameters: `[
{ name: 'source', value: 's3://linz-topographic-upload/abc/', description: 'Source bucket' },
{ name: 'ticket', value: 'GDE-123', description: 'JIRA Ticket' },
]`,
workflowStatus: `Succeeded`,
});

console.log = originalLog;
let logOutputDict = JSON.parse(logOutput);
// override time
logOutputDict.time = 1724037007216;

assert.deepEqual(logOutputDict, {
time: 1724037007216,
level: 20,
pid: 1,
msg: 'Workflow:Succeeded',
workflowGroup: 'land',
parameters: { source: 's3://linz-topographic-upload/abc/', ticket: 'GDE-123' },
});
});

it('should log workflowGroup as land', () => {
const originalLog = console.log;
let logOutput = [];
console.log = (...args) => logOutput.push(args.join(' '));

runScript({
workflowParameters: `[
{ name: 'source', value: 's3://linz-topographic-upload/abc/'},
]`,
});
console.log = originalLog;
const logOutputDict = JSON.parse(logOutput);

assert.equal(logOutputDict.workflowGroup, 'land');
});

it('should log workflowGroup as sea', () => {
const originalLog = console.log;
let logOutput = [];
console.log = (...args) => logOutput.push(args.join(' '));

runScript({
workflowParameters: `[
{ name: 'source', value: 's3://linz-hydrographic-upload/abc/'},
]`,
});

console.log = originalLog;
const logOutputDict = JSON.parse(logOutput);

assert.equal(logOutputDict.workflowGroup, 'sea');
});

it('should log workflowGroup as unknown', () => {
const originalLog = console.log;
let logOutput = [];
console.log = (...args) => logOutput.push(args.join(' '));

runScript({
workflowParameters: `[
{ name: 'source', value: 's3://linz-bucket/abc/'},
]`,
});

console.log = originalLog;
const logOutputDict = JSON.parse(logOutput);

assert.equal(logOutputDict.workflowGroup, 'unknown');
});
});
15 changes: 15 additions & 0 deletions templates/common/exit.handler.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,27 @@ spec:
let parameters = {};
{{= inputs.parameters.workflow_parameters }}.forEach((pair) => (parameters[pair.name] = pair.value));

function guessWorkflowGroup() {
const source = parameters['source'];
if (!source) {
Copy link
Member

@blacha blacha Aug 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

safer to be typeof source !== 'string' as !1 is false then would error the line below with 1.startsWith

return 'unknown';
}
if (source.startsWith('s3://linz-topographic-upload')) {
return 'land';
}
if (source.startsWith('s3://linz-hydrographic-upload')) {
return 'sea';
}
return 'unknown';
}

console.log(
JSON.stringify({
time: Date.now(),
level: 20,
pid: 1,
msg: 'Workflow:{{inputs.parameters.workflow_status}}',
workflowGroup: guessWorkflowGroup(),
parameters,
}),
);