feat: Add support for Amazon EKS #156

ca-nguyen · 2021-08-17T17:06:03Z

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.

shivlaks

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.rst

src/stepfunctions/steps/service.py

shivlaks · 2021-08-17T19:10:57Z

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)
+        """
+        if wait_for_completion:


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

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

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.

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

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.

shivlaks

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.

ca-nguyen · 2021-08-18T16:27:55Z

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.

Done! Thank you!

wong-a

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?

wong-a · 2021-08-23T22:01:23Z

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)
+        """
+        if wait_for_completion:


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.

src/stepfunctions/steps/service.py

Update documentation Co-authored-by: Adam Wong <[email protected]>

wong-a

☸️

ca-nguyen · 2021-08-25T21:50:06Z

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

StepFunctions-Bot · 2021-09-01T18:16:15Z

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

ca-nguyen · 2021-09-01T22:49:37Z

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 LogOptions 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

shivlaks · 2021-09-01T23:59:26Z

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 LogOptions 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.

wong-a · 2021-09-02T18:34:12Z

src/stepfunctions/steps/service.py

        """
-        if wait_for_completion:


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 added 3 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 from shivlaks and wong-a August 17, 2021 17:56

shivlaks suggested changes Aug 17, 2021

View reviewed changes

Adding eks:call and eks:runJob steps

c81ee56

ca-nguyen requested a review from shivlaks August 17, 2021 23:01

shivlaks previously approved these changes Aug 18, 2021

View reviewed changes

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

95538ac

wong-a reviewed Aug 23, 2021

View reviewed changes

Apply suggestions from code review

1907b7d

Update documentation Co-authored-by: Adam Wong <[email protected]>

ca-nguyen dismissed shivlaks’s stale review via 1907b7d August 25, 2021 21:40

wong-a previously approved these changes Aug 25, 2021

View reviewed changes

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 from shivlaks and wong-a via 9a670ef September 1, 2021 07:54

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

1cef855

ca-nguyen requested review from shivlaks and wong-a September 1, 2021 22:44

shivlaks approved these changes Sep 1, 2021

View reviewed changes

wong-a approved these changes Sep 2, 2021

View reviewed changes

ca-nguyen merged commit f8bbfaf into aws: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

feat: Add support for Amazon EKS #156

feat: Add support for Amazon EKS #156

Uh oh!

Conversation

ca-nguyen commented Aug 17, 2021 • edited Loading 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!

shivlaks Aug 17, 2021

Choose a reason for hiding this comment

Uh oh!

ca-nguyen Aug 17, 2021

Choose a reason for hiding this comment

Uh oh!

shivlaks Aug 18, 2021

Choose a reason for hiding this comment

Uh oh!

ca-nguyen Aug 18, 2021

Choose a reason for hiding this comment

Uh oh!

wong-a Aug 23, 2021

Choose a reason for hiding this comment

Uh oh!

shivlaks left a comment • edited Loading 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 commented Aug 18, 2021

Uh oh!

wong-a left a comment

Choose a reason for hiding this comment

Uh oh!

wong-a Aug 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 commented Aug 25, 2021

Uh oh!

StepFunctions-Bot commented Sep 1, 2021

AWS CodeBuild CI Report

Uh oh!

ca-nguyen commented Sep 1, 2021

Uh oh!

shivlaks commented Sep 1, 2021

Uh oh!

wong-a Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ca-nguyen commented Aug 17, 2021 •

edited

Loading

shivlaks left a comment •

edited

Loading