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: sfn delete workflow (with prod token validation and messaging) #1379

Merged
merged 6 commits into from
Jun 20, 2023

Conversation

saikonen
Copy link
Collaborator

Would prefer if original author would incorporate these into their own branch so #1284 can be merged. Otherwise pick one of the following strategies:

@saikonen saikonen requested a review from savingoyal April 27, 2023 13:04
@@ -60,6 +60,19 @@ def _set(self):
],
)

def delete(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

any specific reasons to use delete vs disable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

main reason would be for cleanup purposes, so deleted flows do not leave any residual resources in AWS. A disabled rule would not incur any costs so impact is the same as with delete.

what would the case be for preferring disable over delete? My expectation for how step-functions delete command works would be that resources are removed, not simply disabled.

"*%s*." % (name, prev_user)
)
obj.echo(
"To deploy a new version of this flow, you need to use "
Copy link
Collaborator

Choose a reason for hiding this comment

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

this wouldn't work for deletes. also, it is likely that we may have more commands that wouldn't allow us to combine the token instructions into one. Maybe it is indeed okay to have some repetition.

Copy link
Collaborator Author

@saikonen saikonen Apr 28, 2023

Choose a reason for hiding this comment

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

seems I simply forgot to make this line dynamic, the same output works in the argo-workflows delete/terminate feature already.

refactored this so token usage instructions can be passed to the validator as a function instead, keeping the validate_token() reusable for future commands as well (such as terminate etc. if we end up implementing those). Should be a somewhat cleaner approach now, let me know what you think :)

Copy link
Collaborator

@savingoyal savingoyal left a comment

Choose a reason for hiding this comment

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

few comments. I will test out the PR once I am back in office.

@savingoyal
Copy link
Collaborator

we can make this part of the next release train

Copy link
Collaborator

@savingoyal savingoyal left a comment

Choose a reason for hiding this comment

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

small nit - otherwise good to ship

@savingoyal savingoyal merged commit 9b86a2f into master Jun 20, 2023
@savingoyal savingoyal deleted the feat/sfn-delete-workflow-2 branch June 20, 2023 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants