-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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(lambda-layer-kubectl): depend on @awscdk/asset-kubectl-v20 and reduce aws-cdk-lib size #22677
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
It doesn't seem to be showing the actual build logs here properly, but it looks like you need to add the yarn.lock changes:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, shouldn't this also modify aws-cdk-lib
's package.json?
There was a problem hiding this 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.
✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.
Pretty sure because of #22716 you'll just need to rebase and rerun |
cdkCommandOptions: { | ||
deploy: { | ||
args: { | ||
rollback: true, | ||
}, | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 tests were in need of update in stepfunctions-tasks, because they use a CDK construct that depends on lambda-layer-kubectl, and we updated the asset hash for the layer. cdk deploy
has a normal default of rollback: true
, but our test runner defaults to rollback: false
. According to @corymhall, this is because of the assertion stack; we want them all to run even if some of them fail. There are no assertions here, so I feel safe turning on rollback: true
.
However, calling this out: with rollback: false
, the update workflow fails with Replacement type updates not supported on stack with disable-rollback
on the KubectlLayer
. It looks like on update, the different asset hash requires replacement of the lambda layer. And any sort of replacement is not allowed when rollback: false
, a rule that comes from cloudformation and not the resource itself. I think we should be okay with this, but I definitely want a second opinion. @madeline-k, @rix0rrr, what do you think?
@Mergifyio refresh |
✅ Pull request refreshed |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
This PR reduces the size of aws-cdk-lib by 25.6 MB, by extracting the
lambda-layer-kubectl/lib/layer.zip
file to a separate package thataws-cdk-lib
will directly depend on.This change also reduce the complexity of the build in the aws-cdk repo because the process of building the layer.zip inside of a Docker container has also been extracted to the separate package. See https://github.com/cdklabs/awscdk-asset-kubectl/tree/kubectl-v20/main
All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integ
to deploy the infrastructure and generate the snapshot (i.e.yarn integ
without--dry-run
)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license