-
Notifications
You must be signed in to change notification settings - Fork 41
[Add] workflow template using workflow CLI commands #1983
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
base: main
Are you sure you want to change the base?
[Add] workflow template using workflow CLI commands #1983
Conversation
…to run a migration Signed-off-by: Jugal Chauhan <jugaldc@amazon.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1983 +/- ##
=========================================
Coverage 78.39% 78.39%
Complexity 21 21
=========================================
Files 603 603
Lines 24180 24180
Branches 1844 1844
=========================================
Hits 18955 18955
Misses 4328 4328
Partials 897 897
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
...s/packages/migration-workflow-templates/src/workflowTemplates/workflowCommandOrchestrator.ts
Outdated
Show resolved
Hide resolved
...s/packages/migration-workflow-templates/src/workflowTemplates/workflowCommandOrchestrator.ts
Outdated
Show resolved
Hide resolved
…to run a migration Signed-off-by: Jugal Chauhan <jugaldc@amazon.com>
Signed-off-by: Jugal Chauhan <jugaldc@amazon.com>
…kins-k8s-local-test
Signed-off-by: Jugal Chauhan <jugaldc@amazon.com>
Signed-off-by: Jugal Chauhan <jugaldc@amazon.com>
Signed-off-by: Jugal Chauhan <jugaldc@amazon.com>
...ionSpecs/packages/migration-workflow-templates/src/workflowTemplates/fullMigrationWithCli.ts
Outdated
Show resolved
Hide resolved
...ionSpecs/packages/migration-workflow-templates/src/workflowTemplates/fullMigrationWithCli.ts
Outdated
Show resolved
Hide resolved
...s/packages/migration-workflow-templates/src/workflowTemplates/workflowCommandOrchestrator.ts
Outdated
Show resolved
Hide resolved
orchestrationSpecs/packages/migration-workflow-templates/tests/fullMigrationWithCli.test.ts
Outdated
Show resolved
Hide resolved
...trationSpecs/packages/migration-workflow-templates/tests/workflowCommandOrchestrator.test.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Jugal Chauhan <jugaldc@amazon.com>
Signed-off-by: Jugal Chauhan <jugaldc@amazon.com>
Signed-off-by: Jugal Chauhan <jugaldc@amazon.com>
540831e to
0392e0b
Compare
0392e0b to
a421219
Compare
Signed-off-by: Jugal Chauhan <jugaldc@amazon.com>
a421219 to
c1142b1
Compare
| popd | ||
| } | ||
| apply_workflows "workflows" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this approach - this makes it easy to have a separate production workflows directory and a separate test one (testWorkflows). That solves some of the other gradle directory collision issues that I was describing previously.
| import {CreateSnapshot} from "./createSnapshot"; | ||
| import {DocumentBulkLoad} from "./documentBulkLoad"; | ||
| import {FullMigration} from "./fullMigration"; | ||
| import {testMigrationWithWorkflowCli} from "./testMigrationWithWorkflowCli"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the casing to be consistent
| import {CommonWorkflowParameters} from "./commonUtils/workflowParameters"; | ||
| import {makeRequiredImageParametersForKeys} from "./commonUtils/imageDefinitions"; | ||
|
|
||
| const configComponentParameters = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO (can be for later): We'll need to add http auth (see here)
| { | ||
| "snapshotConfig": { | ||
| "snapshotNameConfig": { | ||
| "snapshotNamePrefix": "source1snapshot1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably include the string "test" or "workflowtest" somehow so that if we ever run this on a cluster that wasn't meant to be a test cluster, one would understand where it came from.
You can also include a workflow.uid or name w/ expr.getWorkflowValue("uid") (here) and see the argo docs) for which variables are supported
| WORKFLOW_OUTPUT=$(workflow submit 2>&1) | ||
| echo "Workflow submit output: $WORKFLOW_OUTPUT" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - would be safer to just dump this directly to stdout rather than to a variable
| echo "$STATUS_OUTPUT" | ||
| RESULT="ERROR" | ||
| EXIT_CODE=2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename this to something like - MONITOR_SHOULD_CONTINUE (TRUE/FALSE) and then return 0 or non-zero at the end.
You could also just remove the variable and exit w/ 0 when RESULT="SUCCESS" or "FAILURE" (and let all others retry).
| .addCommand(["/bin/sh", "-c"]) | ||
| .addResources(DEFAULT_RESOURCES.MIGRATION_CONSOLE_CLI) | ||
| .addArgs([WORKFLOW_MONITOR_SCRIPT]) | ||
| .addPathOutput( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where/how do you use this output? I don't see any other occurrence of monitorResult - I would expect to see it in testRunMigration to check to see if that template itself should return success or failure.
| ) | ||
| .addRetryParameters({ | ||
| limit: "864", // ~72 hours (864 * 5min max backoff) | ||
| retryPolicy: "Always", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to return with a failure and not retry when there's a permanent failure. There are two ways to solve this. One is to add another task to the step here, grabbing the output parameter and testing it (run a failure template conditionally). The other would be to make the retry smarter to retry only transient errors.
Transient errors are defined here - but I wouldn't suggest using that because the definition of a transient error is global, so the test change would affect production workflows. Just throwing it out there though.
It certainly would be cleaner to tweak the retry - though permanent failures shouldn't be common & adding another container task like this isn't bad.
Here's the failure check and template
- name: explicit-fail
container:
image: busybox
command: [sh, -c, "echo 'Workflow condition failed' && exit 1"]
... from some steps
- - name: fail-workflow
template: explicit-fail
when: "{{steps.validate.outputs.parameters.monitorResult}} == FAILED"
Description
This PR replaces the static workflow definitions with a DSL generated
testMigrationWithWorkflowCliWorkflowTemplate that executes migration workflows via the workflow CLI, aligning Jenkins integration tests with the production deployment model.Changes include
Removed:
Added:
testMigrationWithWorkflowCli.ts- DSL-generated WorkflowTemplate that:workflow statusoutputThis workflow has three main templates:
configureAndSubmitWorkflow- Builds migration config JSON and submits viaworkflow configure+workflow submitmonitorWorkflow- Pollsworkflow status, auto-approves suspended workflows, exits on success/failuretestRunMigration- entrypointIssues Resolved
MIGRATIONS-2750
Testing
None yet
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.