-
Notifications
You must be signed in to change notification settings - Fork 88
feat: Add support for Amazon EKS #156
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
Conversation
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.
thanks for adding these!! looks great for a first cut.
had some minor nits I think we should address. take a look and let me know if you have any questions!
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.
question: with defaults, I know we're being consistent by setting the default to be synchronous, but I wonder if that's the most pragmatic/idiomatic thing.
not asking for changes, just pondering out loud for now
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.
Good question - you're right this was done this way for consistency.
A point to consider: If we were to change the defaults, we would need to notify of the change of behaviour as this might cause breaking change for our customers. Not sure if this is worth bumping the version
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.
we haven't set defaults for these steps yet as they're unreleased, so now is the time to make that decision. depending on API/service, I think it'd be sensible to consider alternatives...
somewhat related: I'm quickly writing up the StepFunctions startExecution integration step... and that supports all 3 (wait for completion, wait for task token, request/response)... what would be a sensible default there? all the other service integrations only offer 2 options.
I think this is fine to leave it as is for now for Eks - if we were to change defaults, it would need to go with the next major version bump.
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.
My last comment, might not have been clear, but we are on the same page! :)
Let's keep the defaults as is to be consistent with the other released steps for now
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 expect wait_for_completion=True is what most customers usually want. Since all other steps supporting .sync use it by default, we should probably be consistent so there are less surprises.
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.
go for it - please be sure to update the PR description to include runJob and call APIs that were added in the most recent commit.
go for it - please be sure to update the PR description to include
runJobandcallAPIs that were added in the most recent commit.
Done! Thank you!
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.
LGTM besides some minor updates to the docs.
I see there are unit tests, but did you do any manual testing to ensure each step can be created with Step Functions through the SDK?
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 expect wait_for_completion=True is what most customers usually want. Since all other steps supporting .sync use it by default, we should probably be consistent so there are less surprises.
Update documentation Co-authored-by: Adam Wong <55506708+wong-a@users.noreply.github.com>
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.
☸️
LGTM besides some minor updates to the docs.
Thanks for reviewing! I applied your suggested changes
I see there are unit tests, but did you do any manual testing to ensure each step can be created with Step Functions through the SDK?
I did not do any testing outside of unit tests. I'll make sure to manual test before merging
...arasms for eks:runJob
9a670ef
AWS CodeBuild CI Report
- CodeBuild project: AutoBuildProject6AEA49D1-sEHrOdk7acJc
- Commit ID: 1cef855
- Result: SUCCEEDED
- Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
I see there are unit tests, but did you do any manual testing to ensure each step can be created with Step Functions through the SDK?
Tested manually and uncovered a few bugs:
- eks::call does not have option to run a job (.sync)
- removed
LogOptionsparam from eks::runJob example as it does not allow to retrieve of logs (only possible with eks::runJob.sync)
Made the changes in the last commits
I see there are unit tests, but did you do any manual testing to ensure each step can be created with Step Functions through the SDK?
Tested manually and uncovered a few bugs:
- eks::call does not have option to run a job (.sync)
- removed
LogOptionsparam from eks::runJob example as it does not allow to retrieve of logs (only possible with eks::runJob.sync)
Made the changes in the last commits
glad we caught those. be sure to expand on testing methodology and the steps we followed.
not for this PR:
thought: how can we automate that testing (via unit/simple integ tests). It will be helpful to establish so that we responsibly introduce service integrations. More importantly, regressions are easy to introduce if we don't have automated tests in place.
src/stepfunctions/steps/service.py
Outdated
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.
Glad we caught this. The public documentation isn't really clear about which service integrations support .sync.
https://docs.aws.amazon.com/step-functions/latest/dg/connect-supported-services.html
https://docs.aws.amazon.com/step-functions/latest/dg/connect-eks.html
Uh oh!
There was an error while loading. Please reload this page.
Issue #, if available: #106
Description of changes:
Added:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.