aws/aws-step-functions-data-science-sdk-pythonPublic

NotificationsYou must be signed in to change notification settings
Fork89
Star292

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

ca-nguyen merged 8 commits intoaws:mainfromca-nguyen:add-support-for-eks

Sep 2, 2021

Merged

feat: Add support for Amazon EKS#156

ca-nguyen merged 8 commits intoaws:mainfromca-nguyen:add-support-for-eks

Sep 2, 2021

Conversation

Copy link

Contributor

ca-nguyen commentedAug 17, 2021•
edited
Loading

Issue #, if available:#106

Description of changes:
Added:

EksRunJob to create a Task state that allows you to run a job on your Amazon EKS cluster
EksCall to create a Task state that allows you to use the Kubernetes API to read and write Kubernetes resource objects via a Kubernetes API endpoint
EksCreateClusterStep to create a Task state that creates an Amazon EKS cluster
EksDeleteClusterStep to create a Task state that deletes an Amazon EKS cluster
EksCreateFargateProfileStep to create a Task state that creates a Fargate profile
EksDeleteFargateProfileStep to create a Task state that deletes a Fargate profile
EksCreateNodeGroupStep to create a Task state that creates a node group
EksDeleteNodeGroupStep to create a Task state that deletes a node group

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

ca-nguyen added3 commits

June 18, 2021 19:23

Feature: Add support to Amazon EKS service integration

abd6b10

Merged from main

555feea

Removed double import due to merge

52db3de

ca-nguyen requested review fromshivlaks andwong-a

August 17, 2021 17:56

shivlaks suggested changes

Aug 17, 2021

View reviewed changes

Copy link

Contributor

shivlaks left a 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!

doc/services.rstShow resolvedHide resolved

doc/services.rst OutdatedShow resolvedHide resolved

src/stepfunctions/steps/service.py OutdatedShow resolvedHide resolved

src/stepfunctions/steps/service.py

		output_path (str, optional): Path applied to the state’s output after the application of `result_path`, producing the effective output which serves as the raw input for the next state. (default: '$')
		wait_for_completion (bool, optional): Boolean value set to `True` if the Task state should wait to complete before proceeding to the next step in the workflow. (default: True)
		"""
		ifwait_for_completion:

Copy link

Contributor

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

Copy link

ContributorAuthor

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

Copy link

Contributor

shivlaksAug 18, 2021

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.

Copy link

ContributorAuthor

ca-nguyenAug 18, 2021

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

Copy link

Contributor

wong-aAug 23, 2021

Choose a reason for hiding this comment

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

I expectwait_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.

Adding eks:call and eks:runJob steps

c81ee56

ca-nguyen requested a review fromshivlaks

August 17, 2021 23:01

shivlaks previously approved these changes

Aug 18, 2021

View reviewed changes

Copy link

Contributor

shivlaks left a comment•
edited
Loading

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 includerunJob andcall APIs that were added in the most recent commit.

Copy link

ContributorAuthor

ca-nguyen commentedAug 18, 2021

go for it - please be sure to update the PR description to includerunJob andcall APIs that were added in the most recent commit.

Done! Thank you!

Merge branch 'main' into add-support-for-eks

95538ac

wong-a reviewed

Aug 23, 2021

View reviewed changes

Copy link

Contributor

wong-a left a 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?

src/stepfunctions/steps/service.py

		output_path (str, optional): Path applied to the state’s output after the application of `result_path`, producing the effective output which serves as the raw input for the next state. (default: '$')
		wait_for_completion (bool, optional): Boolean value set to `True` if the Task state should wait to complete before proceeding to the next step in the workflow. (default: True)
		"""
		ifwait_for_completion:

Copy link

Contributor

wong-aAug 23, 2021

Choose a reason for hiding this comment

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

I expectwait_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.

src/stepfunctions/steps/service.py OutdatedShow resolvedHide resolved

Apply suggestions from code review

1907b7d

Update documentationCo-authored-by: Adam Wong <55506708+wong-a@users.noreply.github.com>

ca-nguyen dismissedshivlaks’sstale review via1907b7d

August 25, 2021 21:40

wong-a previously approved these changes

Aug 25, 2021

View reviewed changes

Copy link

Contributor

wong-a left a comment

Choose a reason for hiding this comment

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

☸️

Copy link

ContributorAuthor

ca-nguyen commentedAug 25, 2021

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

shivlaks previously approved these changes

Aug 25, 2021

View reviewed changes

Removed eks:call.sync as it is not supported and removed logoptions p…

9a670ef

…arasms for eks:runJob

ca-nguyen dismissed stale reviews fromshivlaks andwong-a via9a670ef

September 1, 2021 07:54

Corrected eks::call and eks::runJob resource arn example

1cef855

Copy link

Contributor

StepFunctions-Bot commentedSep 1, 2021

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-sEHrOdk7acJc
Commit ID:1cef855
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered bygithub-codebuild-logs, available on theAWS Serverless Application Repository

ca-nguyen requested review fromshivlaks andwong-a

September 1, 2021 22:44

Copy link

ContributorAuthor

ca-nguyen commentedSep 1, 2021

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)
removedLogOptions param 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

Copy link

Contributor

shivlaks commentedSep 1, 2021

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)
removedLogOptions param 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.

shivlaks approved these changes

Sep 1, 2021

View reviewed changes

wong-a approved these changes

Sep 2, 2021

View reviewed changes

src/stepfunctions/steps/service.py Outdated

		output_path (str, optional): Path applied to the state’s output after the application of `result_path`, producing the effective output which serves as the raw input for the next state. (default: '$')
		wait_for_completion (bool, optional): Boolean value set to `True` if the Task state should wait to complete before proceeding to the next step in the workflow. (default: True)
		"""
		ifwait_for_completion:

Copy link

Contributor

wong-aSep 2, 2021

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

ca-nguyen merged commitf8bbfaf intoaws:main

Sep 2, 2021

ca-nguyen mentioned this pull request

Sep 2, 2021

Feature Request: EKS support#106

Closed

shivlaks mentioned this pull request

Sep 13, 2021

feat: Add support for nested Aws StepFunctions service integration#166

Merged

6 tasks

ca-nguyen deleted the add-support-for-eks branch

October 27, 2021 01:12

Labels

None yet

Movatterモバイル変換

feat: Add support for Amazon EKS#156

feat: Add support for Amazon EKS#156

Uh oh!

Conversation

ca-nguyen commentedAug 17, 2021• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

shivlaks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shivlaksAug 17, 2021

Choose a reason for hiding this comment

Uh oh!

ca-nguyenAug 17, 2021

Choose a reason for hiding this comment

Uh oh!

shivlaksAug 18, 2021

Choose a reason for hiding this comment

Uh oh!

ca-nguyenAug 18, 2021

Choose a reason for hiding this comment

Uh oh!

wong-aAug 23, 2021

Choose a reason for hiding this comment

Uh oh!

shivlaks left a comment• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ca-nguyen commentedAug 18, 2021

Uh oh!

wong-a left a comment

Choose a reason for hiding this comment

Uh oh!

wong-aAug 23, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wong-a left a comment

Choose a reason for hiding this comment

Uh oh!

ca-nguyen commentedAug 25, 2021

Uh oh!

StepFunctions-Bot commentedSep 1, 2021

AWS CodeBuild CI Report

Uh oh!

ca-nguyen commentedSep 1, 2021

Uh oh!

shivlaks commentedSep 1, 2021

Uh oh!

wong-aSep 2, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ca-nguyen commentedAug 17, 2021•
edited
Loading

shivlaks left a comment•
edited
Loading