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(stepfunctions-tasks): add AutoTerminationPolicy to EmrCreateCluster #16976

Merged
merged 12 commits into from
Nov 9, 2021

Conversation

jacobfi
Copy link
Contributor

@jacobfi jacobfi commented Oct 14, 2021

Auto-termination is supported with Amazon EMR versions 5.30.0 and 6.1.0 and later.
This change enables CDK users to specify an auto-termination policy on EMR clusters created through the aws-stepfunctions task EmrCreateCluster.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@gitpod-io
Copy link

gitpod-io bot commented Oct 14, 2021

@kaizencc kaizencc self-assigned this Oct 18, 2021
@peterwoodworth peterwoodworth changed the title feat(aws-stepfunctions-tasks): added AutoTerminationPolicy property to EmrCreateCluster step feat(aws-stepfunctions-tasks): added AutoTerminationPolicy property to EmrCreateCluster step Oct 21, 2021
@kaizencc kaizencc added effort/small Small work item – less than a day of effort p1 labels Oct 22, 2021
Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

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

Regarding the comment below, this does not seem like it is possible to do in the CDK. The CDK generates a template that gets consumed by CloudFormation to provision resources, so if the property is not in the CloudFormation it is not something we can do.

Have you tested your code in an integration test? I have a feeling that the AutoTerminationPolicy, though it shows up in the template (and thus your test passes), does not actually do anything come deploy time.

Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

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

This is looking great @jacobfi! Just a few minor changes.

@mergify mergify bot dismissed kaizencc’s stale review October 28, 2021 13:07

Pull request has been modified.

kaizencc
kaizencc previously approved these changes Nov 4, 2021
Copy link
Contributor

@kaizencc kaizencc 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 the contribution @jacobfi!

@mergify
Copy link
Contributor

mergify bot commented Nov 4, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@kaizencc kaizencc changed the title feat(aws-stepfunctions-tasks): added AutoTerminationPolicy property to EmrCreateCluster step feat(stepfunctions-tasks): add AutoTerminationPolicy to EmrCreateCluster Nov 4, 2021
@kaizencc
Copy link
Contributor

kaizencc commented Nov 4, 2021

@jacobfi to merge this, we need to get past this error:

The merge-queue pull request can't be update
Details: `Pull request can't be updated with latest base branch changes

Mergify needs the author permission to update the base branch of the pull request.
audienceproject needs to authorize modification on its head branch.
err-code: C1535`

@mergify
Copy link
Contributor

mergify bot commented Nov 4, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@jacobfi
Copy link
Contributor Author

jacobfi commented Nov 5, 2021

@jacobfi to merge this, we need to get past this error:

The merge-queue pull request can't be update
Details: `Pull request can't be updated with latest base branch changes

Mergify needs the author permission to update the base branch of the pull request.
audienceproject needs to authorize modification on its head branch.
err-code: C1535`

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Hi @kaizen3031593

Thank you for approving the PR.

I would love to give your mergify bot access to the fork, but I can't seem to make it work. I have followed the link to the Github doc explaining how, but the option of "Allow edits from maintainers" is nowhere to be seen on this PR.

I can add people and teams manually to access the fork, but I don't know which users/teams to add. Can you help with that?
I can find some users with the name 'mergify' but something tells me that's not your bot.

@jacobfi
Copy link
Contributor Author

jacobfi commented Nov 5, 2021

@kaizen3031593
I have added you as admin on the fork.

@kaizencc
Copy link
Contributor

kaizencc commented Nov 5, 2021

@jacobfi unfortunately that doesn't help. We really need that "allow edits from maintainers" tag to be set. I can think of two things to do: perhaps somewhere in your settings you have set something to never allow this option? Alternatively you can try to open a new pull request and there should be a button that says "allow edits from maintainers" that is by default set to true. See if that's the case for you, and if so, you can just open a new PR with that tag already set.

@jacobfi
Copy link
Contributor Author

jacobfi commented Nov 8, 2021

Hello @kaizen3031593
I think I found a solution by transfering the repository to my own Github user.
You can proceed with the merge - it should work now!

@mergify mergify bot dismissed kaizencc’s stale review November 8, 2021 22:11

Pull request has been modified.

@mergify
Copy link
Contributor

mergify bot commented Nov 8, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 27ad7d8 into aws:master Nov 9, 2021
@mergify
Copy link
Contributor

mergify bot commented Nov 9, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: f773cbd
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

kaizencc added a commit that referenced this pull request Nov 11, 2021
mergify bot pushed a commit that referenced this pull request Nov 12, 2021
This reverts commit: #16976.

`AutoTerminationPolicy` was never supported by stepfunctions,
even though it is supported by EMR. This was not discovered 
until it was used since the documentation does not mention this
limitation of stepfunctions.

Since it never worked, we are okay with the breaking change here.

Closes #17452.
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
…luster` (aws#16976)

Auto-termination is supported with Amazon EMR versions 5.30.0 and 6.1.0 and later.
This change enables CDK users to specify an auto-termination policy on EMR clusters created through the aws-stepfunctions task EmrCreateCluster.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
This reverts commit: aws#16976.

`AutoTerminationPolicy` was never supported by stepfunctions,
even though it is supported by EMR. This was not discovered 
until it was used since the documentation does not mention this
limitation of stepfunctions.

Since it never worked, we are okay with the breaking change here.

Closes aws#17452.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-stepfunctions-tasks effort/small Small work item – less than a day of effort p1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants