-
Notifications
You must be signed in to change notification settings - Fork 4k
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(cli): remove deprecated validation around ARM build image types #30879
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.
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.
223785f
to
b820a2c
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
This looks like a good start, but still needs a couple things:
- Do we call the validate function anywhere? We should clean up that invocation.
- There are a lot of changes to these test files that are spacing or changing ' to ". Let's not change the formatting, and only remove the impacted tests.
- This would be a great change to add in an integ test for some of the other instance sizes. They are in
packages/@aws-cdk-testing/framework-integ
. If there are codebuilds with ARM images, lets add one in the same test file where the compute type was previously disallowed. That way we have verification that these sizes are indeed supported!
@scanlonp , thanks for the feedback here. Working on this today and had a few questions:
Other that those two files I didn't see other mentions of this specific implementation or calling of
|
@PeterEckIII, I may have been wrong about removing the validate function. It does need to be present and implemented to correctly implement Also, would you mind amending the title of the PR? Thankfully we are fixing codebuild, not the cli :) |
@scanlonp, oh no! 😅 Ok, in that case I may withdraw the PR altogether and just start fresh if that's ok. |
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. |
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. |
Comments on closed issues and PRs are hard for our team to see. |
The pull request linter fails with the following errors:
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 |
Issue # (if applicable)
Closes #30869 .
Reason for this change
Now that ARM images support all compute types (not just
ComputeType.SMALL
andComputeType.LARGE
) the validate function is outdated.Description of changes
Removed the
validate
method fromaws-cdk/packages/aws-cdk-lib/aws-codebuild/lib/linux-arm-build-image.ts
and the accompanying commentDescription of how you validated changes
Updated tests for
aws-codebuild
to reflect the accepted types.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license