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.

Sign up for GitHub

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

Jump to bottom

add ability to terminate execution of a step-fn state machine #1695

Merged
merged 5 commits into from
Jan 30, 2024
Merged

add ability to terminate execution of a step-fn state machine #1695

merged 5 commits into from
Jan 30, 2024

Conversation

madhur-ob
Copy link
Collaborator

This PR should over-ride #1596

Usage:
python flow.py step-functions terminate sfn-d41004cd-1bbf-4c25-b284-94bfc921d78b

@madhur-ob
Copy link
Collaborator Author

Looking at test failures...

saikonen
saikonen reviewed Jan 24, 2024
View reviewed changes
Copy link
Collaborator

@saikonen saikonen left a comment

Choose a reason for hiding this comment

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

had a quick look so far, a few comments but overall looking good. I'll take this for a spin tomorrow when I have more time.

metaflow/plugins/argo/argo_workflows.py Show resolved Hide resolved
metaflow/plugins/aws/step_functions/step_functions.py Show resolved Hide resolved
@madhur-ob madhur-ob requested a review from saikonen January 29, 2024 21:38
@madhur-ob
Copy link
Collaborator Author

@saikonen can you confirm if you don't need the --authorize flag if you are trying to terminate as another user.
If you confirm, then we need to discuss -- since this is probably a bug or something that I am not aware of...

@saikonen
Copy link
Collaborator

@saikonen can you confirm if you don't need the --authorize flag if you are trying to terminate as another user. If you confirm, then we need to discuss -- since this is probably a bug or something that I am not aware of...

tested this and the token authorization and user/owner checks are working correctly. The issue with testing this is that the production token is cached locally, so in order to test a case where both user and token do not match, you need to invalidate the locally cached token

@nflx-mf-bot
Copy link
Collaborator

Testing[619] @ d7400b1

savingoyal
savingoyal reviewed Jan 30, 2024
View reviewed changes
metaflow/plugins/aws/step_functions/step_functions_cli.py
@@ -614,6 +618,83 @@ def _token_instructions(flow_name, prev_user):
)


@step_functions.command(help="Terminate flow execution on Step Functions.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor nit - but for consistency AWS Step Functions. not blocking for merging this PR in, but would be good if you can sweep this and various CLIs to polish up the help strings and make them consistent.

@savingoyal savingoyal merged commit 81d8909 into Netflix:master Jan 30, 2024
32 of 35 checks passed
@madhur-ob madhur-ob mentioned this pull request Feb 5, 2024
Closed
@madhur-ob madhur-ob deleted the aws-step-fn-terminate branch February 5, 2024 20:15
@madhur-ob madhur-ob mentioned this pull request Feb 27, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@savingoyal savingoyal savingoyal left review comments

@saikonen saikonen saikonen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@madhur-ob @saikonen @nflx-mf-bot @savingoyal