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.

Sign up for GitHub

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

Jump to bottom

feat(eks): Add atomic flag for helm construct #24583

Closed
wants to merge 9 commits into from
Closed

feat(eks): Add atomic flag for helm construct #24583

wants to merge 9 commits into from

Conversation

alexandersperling
Copy link
Contributor

@alexandersperling alexandersperling commented Mar 12, 2023 •
edited
Loading

This PR adds the possibilty to use the atomic flag for helm charts.

Basically I took code from an PR which was closed sometime ago, refactored it and made changes for an integration test.

  • Add atomic flag for helm chart construct
  • Update unit test
  • Update README
  • Update integ test

Closes #22254

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

@github-actions github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 labels Mar 12, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team March 12, 2023 18:12
aws-cdk-automation
aws-cdk-automation previously requested changes Mar 12, 2023
View reviewed changes
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.

@alexandersperling
Copy link
Contributor Author

alexandersperling commented Mar 12, 2023 •
edited
Loading

Clarification Request

I tried running integration test, but got an error while deploying, which relates to the skip-crd helm chart.

I did not run the integration test updates when adding the skip-crd flag, so I'm not sure how they passed the last time.

Could someone take a look into this, rerun the integration tests and let me know, if any changes are needed for this?

Failed resources:
aws-cdk-eks-helm-test | 18:35:10 | CREATE_FAILED        | Custom::AWSCDK-EKS-HelmChart          | Cluster/chart-test-skip-crd-installation/Resource/Default (ClustercharttestskipcrdinstallationB8323954) Received response status [FAILED] from custom resource. Message returned: Error: b'Release "lambda-chart-release" does not exist. Installing it now.\nError: release: already exists\n'

@alexandersperling alexandersperling changed the title feat (eks): Add atomic flag for helm construct feat(eks): Add atomic flag for helm construct Mar 12, 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 12, 2023
@TheRealAmazonKendra
Copy link
Contributor

TheRealAmazonKendra commented Mar 30, 2023 •
edited
Loading

Clarification Request

I tried running integration test, but got an error while deploying, which relates to the skip-crd helm chart.

I did not run the integration test updates when adding the skip-crd flag, so I'm not sure how they passed the last time.

Could someone take a look into this, rerun the integration tests and let me know, if any changes are needed for this?

Failed resources:
aws-cdk-eks-helm-test | 18:35:10 | CREATE_FAILED        | Custom::AWSCDK-EKS-HelmChart          | Cluster/chart-test-skip-crd-installation/Resource/Default (ClustercharttestskipcrdinstallationB8323954) Received response status [FAILED] from custom resource. Message returned: Error: b'Release "lambda-chart-release" does not exist. Installing it now.\nError: release: already exists\n'

What do you mean by you didn't run the integration test updates last time? Our integ tests work by deploying the pre-change template and then update it with the changes applied. If you didn't run the update, that test just plain never worked.

Also each test shouldn't contain multiple test cases so these should have each been individual tests anyway.

@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 30, 2023
@alexandersperling
Copy link
Contributor Author

What do you mean by you didn't run the integration test updates last time? Our integ tests work by deploying the pre-change template and then update it with the changes applied. If you didn't run the update, that test just plain never worked.

Thats what I thought as well.
In #24213 I added the flag for skipping crds and were not able to run integration tests (no bootstraped AWS account).
However I added a case in the integ test file and you were running the tests for me which updated the snapshots. Thats why I said, 'I did not run the integration test updates' and 'I'm not sure how they passed the last time.'.

Also each test shouldn't contain multiple test cases so these should have each been individual tests anyway.

Okay, so I should better add separate test files for both of these cases? I was just adding them to the integ.eks-helm-asset.ts because it already contained more than one case, but if it's the preferred way to have separate files I can do that.

@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

1 similar comment
@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@aws-cdk-automation
Copy link
Collaborator

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

@alexandersperling
feat: Allow atomic flag for helm construct
ce5c604 
* Add atomic flag for helm chart construct
* Update unit test
* Update README
* Update integ test
@alexandersperling
Rebase
bd6c789
@aws-cdk-automation aws-cdk-automation dismissed their stale review April 3, 2023 08:11

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

@gitpod-io
Copy link

gitpod-io bot commented Apr 21, 2023

@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

2 similar comments
@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation
aws-cdk-automation previously requested changes Apr 28, 2023
View reviewed changes
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.

@aws-cdk-automation aws-cdk-automation dismissed their stale review April 28, 2023 15:15

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

@alexandersperling
Copy link
Contributor Author

Clarification Request

Hi, I've updated the corresponding integ test file, but unfortunately I'm not able to run an integtest and update the snapshot.
Would be great if someone take this over, like described in the CONTRIBUTING.md

@aws-cdk-automation aws-cdk-automation added pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels May 2, 2023
@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

7 similar comments
@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@corymhall corymhall removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label May 23, 2023
@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label May 24, 2023
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 547e79d
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@corymhall corymhall removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label May 24, 2023
@jeffb4
Copy link
Contributor

jeffb4 commented Jun 14, 2023

@TheRealAmazonKendra I pulled up the codebuild logs from the last build and was unable to determine what had failed. There were a lot of docker/moby warnings and errors spamming the logs, was that the reason a failure was registered?

@aws-cdk-automation
Copy link
Collaborator

This PR has been in the BUILD FAILING state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

@aws-cdk-automation
Copy link
Collaborator

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

@aws-cdk-automation aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Jun 22, 2023
@alexandersperling alexandersperling deleted the feat/helm-flag-atomic branch December 12, 2023 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aws-cdk-automation aws-cdk-automation aws-cdk-automation left review comments

Assignees
No one assigned
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

aws-eks: support --atomic flag for helm commands
5 participants
@alexandersperling @TheRealAmazonKendra @aws-cdk-automation @jeffb4 @corymhall