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

chore(eks): Add missing ami types for node groups #24830

Merged
merged 25 commits into from
Apr 25, 2023
Merged

Conversation

cgamache
Copy link
Contributor

Adds Windows amiTypes in aws-eks. These are supported via Cloudformation but missing in cdk since the Enum is manually defined.

Closes #24803


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

Adds Windows amiTypes in aws-eks. These are supported via Cloudformation but missing in cdk since the Enum is manually defined.

Fixes aws#24803
@github-actions github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Mar 28, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team March 28, 2023 17:21
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@cgamache
Copy link
Contributor Author

cgamache commented Mar 28, 2023

Clarification Request: I am not able to run integration tests. Please advise how the README should change.

@cgamache cgamache changed the title feat(aws-eks): Add missing ami types for node groups feat(eks): Add missing ami types for node groups Mar 28, 2023
@aws-cdk-automation aws-cdk-automation added the pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run label Mar 28, 2023
@TheRealAmazonKendra
Copy link
Contributor

Clarification Request: I am not able to run integration tests. Please advise how the README should change.

This should actually be a chore not a feat. Those requirements will disappear.

@cgamache cgamache changed the title feat(eks): Add missing ami types for node groups chore(eks): Add missing ami types for node groups Mar 28, 2023
@TheRealAmazonKendra TheRealAmazonKendra removed the pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run label Mar 28, 2023
@aws-cdk-automation aws-cdk-automation dismissed their stale review March 28, 2023 19:40

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

@cgamache
Copy link
Contributor Author

Hi @TheRealAmazonKendra ! ... Thanks for the tip from before. The build passed, and it now only lacks an approving review.

Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

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

I'm in agreement with @otaviomacedo here about moving the logic inside InstanceType. While we may have another location that does what you've done here, it's not a great design choice and we should have caught it in that instance as well.

On second glance at this, we actually have more complexity in the logic about which ami's work where so my original comment was wrong in thinking that we didn't need integ tests here. Apologies for not looking closer in the first place and providing misleading information.

Since you mentioned that you aren't able to run integration tests, we'll run them for you. We just need you to write the test cases and we'll run them and update the snapshots.

@mergify mergify bot dismissed TheRealAmazonKendra’s stale review April 7, 2023 19:04

Pull request has been modified.

@cgamache
Copy link
Contributor Author

cgamache commented Apr 7, 2023

PR updated with requested changes, @TheRealAmazonKendra ... Please let me know if there is anything that I missed.

🤞 this should be ready for integration tests.

Copy link
Contributor

@otaviomacedo otaviomacedo left a comment

Choose a reason for hiding this comment

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

Thanks, @cgamache. Please go ahead and write the integration test cases, so we can generate the snapshots for you.

@mergify mergify bot dismissed otaviomacedo’s stale review April 11, 2023 15:37

Pull request has been modified.

@cgamache
Copy link
Contributor Author

@otaviomacedo @TheRealAmazonKendra Integration test written and added to the PR.

Bad news though-- the build is now failing. I'm not sure if the un-run integration test is the culprit. It builds and unit tests locally without failures.

Nothing looks out of place in the build logs until the failure at the end. And the failure message isn't specific as to what has gone wrong. I do see that it noticed the "NEW" integration test.

Please advise. Thanks very much!

@gitpod-io
Copy link

gitpod-io bot commented Apr 20, 2023

@mergify
Copy link
Contributor

mergify bot commented Apr 25, 2023

Thank you for contributing! Your pull request will be updated from main 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: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: f3ef17a
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit d82c6c9 into aws:main Apr 25, 2023
@mergify
Copy link
Contributor

mergify bot commented Apr 25, 2023

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

madeline-k pushed a commit that referenced this pull request Apr 27, 2023
Adds Windows amiTypes in aws-eks. These are supported via Cloudformation but missing in cdk since the Enum is manually defined.

Closes #24803

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws-eks: Add missing ami types for node groups
4 participants