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

fix(eks): cluster.addHelmChart ignores skipCrds #31832

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dancmeyers
Copy link

Issue # (if applicable)

Closes #31831.

Reason for this change

The kubectl custom resource lambda was not correctly passing the skipCrds parameter of aws_eks.Cluster.addHelmChart all the way through to the invocation of helm. As such, unexpected behaviour was observed where, even if skipCrds was true, helm running within the lambda would still attempt to install any relevant CRDs as part of the chart.

Description of changes

Adds the skip_crds argument to the call to helm within the helm_handler function.

Description of how you validated changes

I've run the unit tests and verified no changes. I cannot run the integration tests as I cannot afford the cost of deploying a full EKS cluster into my personal AWS account and I can't link this PR to our org's GitHub as per your guidelines, so I can't deploy into an account our org owns through CI.

Checklist


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

fixes: aws#31831

The argument was not being passed from the `helm_handler` calling
`helm`.
@github-actions github-actions bot added bug This issue is a bug. p2 labels Oct 21, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team October 21, 2024 11:17
@github-actions github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Oct 21, 2024
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.

@dancmeyers dancmeyers changed the title fix(aws-eks): cluster.addHelmChart ignores skipCrds fix(eks): cluster.addHelmChart ignores skipCrds Oct 21, 2024
@dancmeyers
Copy link
Author

Exemption Request

You ask that integration test snapshots are not manually updated but are generated by a real deploy. I am unable to do that. Can someone with commit privileges on this PR please run that step and update accordingly.

@aws-cdk-automation aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Oct 21, 2024
@dancmeyers
Copy link
Author

Clarification Request

I have found some very minimal tests for the existing lambda handler, under packages/aws-cdk-lib/lambda-layer-kubectl/test. I am unsure how to best test my change in a way acceptable to the CDK team as I cannot find any examples to build from. Please provide guidance. Should I mock the helm function and make sure that all the args are provided by helm_handler? How would you prefer that be done in Python?

@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 Oct 21, 2024
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 886674f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@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 Oct 21, 2024
@aws-cdk-automation
Copy link
Collaborator

The pull request linter fails with the following errors:

❌ Fixes must contain a change to a test file.
❌ Fixes must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

✅ A exemption request has been requested. Please wait for a maintainer's review.

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 bug This issue is a bug. p2 pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@aws-cdk/aws-eks: cluster.addHelmChart doesn't pass SkipCrds
2 participants