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

feat(codebuild): improved support for ARM build images #19052

Merged
merged 37 commits into from Mar 7, 2022
Merged

feat(codebuild): improved support for ARM build images #19052

merged 37 commits into from Mar 7, 2022

Conversation

tmmvn
Copy link
Contributor

@tmmvn tmmvn commented Feb 19, 2022

Fixes #18916
Fixes #9817

Motivation

CDK currently has poor and hidden support for using ARM build images for CodeBuild that do not match what you can do with the Console. Currently, CDK has under LinuxBuildImage two constants not mentioned in the documentation. The constants internally map to a hidden ArmBuildImage class, which provides support for the standard CodeBuild ARM build images. That is the extent of the support, making ARM a second class citizen compared to x86-64 Linux and Windows build images as, for example, you can't use custom aarch64 ECR images.

Changes

This pull request addresses the missing support by:

  • renaming the previously hidden class ArmBuildImage to LinuxArmBuildImage (in case there are Windows ARM Build Images in the future).
  • exporting LinuxArmBuildImage so it can be used.
  • adding the two ARM constants present in LinuxBuildImage also to LinuxArmBuildImage. The constants are also left under LinuxBuildImage to not break backwards compatibility.
  • adding the method fromEcrRepository() to support custom ARM build images.
  • making the LinuxArmBuildImage closer to the LinuxBuildImage and WindowsBuildImage (built with props instead of just image name).
  • updating documentation to show examples of ARM and highlighting the LinuxBuildImage is for x86-64.

Testing

The unit test for ARM image is still valid.

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

…y supported by CDK by:

- renaming the previously hidden class ArmBuildImage to LinuxArmBuildImage (in case there are Windows ARM Build Images in the future).
- exporting LinuxArmBuildImage so it can be used.
- adding the two ARM constants present in LinuxBuildImage also to LinuxArmBuildImage.
- adding the method fromEcrRepository to support custom ARM build images.
- making the LinuxArmBuildImage more akin to the LinuxBuildImage and WindowsBuildImage (built with props instead of just image name).
- updating documentation to show examples of ARM and highlighting the LinuxBuildImage is for x86-64.
@gitpod-io
Copy link

gitpod-io bot commented Feb 19, 2022

@github-actions github-actions bot added the @aws-cdk/aws-codebuild Related to AWS CodeBuild label Feb 19, 2022
@skinny85 skinny85 changed the title feat(aws-codebuild): add better support for arm build images feat(codebuild): improve support for ARM build images Feb 19, 2022
@skinny85
Copy link
Contributor

@codemeddler any chance of adding a few unit tests? 🙂

@tmmvn
Copy link
Contributor Author

tmmvn commented Feb 19, 2022

@codemeddler any chance of adding a few unit tests? 🙂

Sure if you can point me where to add them and which ones you want. I saw the GPU ones had their own file, the codebuild.test has ARM image testing in it, and only one for the fromEcrRepository is in integ.ecr.lit.

@skinny85
Copy link
Contributor

Let's create a new file, linux-arm-build-image.test.ts, and model it after the existing linux-gpu-build-image.test.ts file.

@tmmvn
Copy link
Contributor Author

tmmvn commented Feb 23, 2022

@skinny85 Does it matter which values are used for the image repositories in the tests? I can see the public ECR registry has many foobar repositories, and not sure which one the GPU one points to, or if it matters what it points to.

@skinny85
Copy link
Contributor

skinny85 commented Feb 23, 2022

The tests should use the constants defined in the new class. If you're asking about testing the fromEcrRepository() method - no, it doesn't have to use real images. Any ECR repo, including a fake "imported" one (Repository.fromRepositoryName()), is fine for these tests.

…y supported by CDK by:

- renaming the previously hidden class ArmBuildImage to LinuxArmBuildImage (in case there are Windows ARM Build Images in the future).
- exporting LinuxArmBuildImage so it can be used.
- adding the two ARM constants present in LinuxBuildImage also to LinuxArmBuildImage.
- adding the method fromEcrRepository to support custom ARM build images.
- making the LinuxArmBuildImage more akin to the LinuxBuildImage and WindowsBuildImage (built with props instead of just image name).
- updating documentation to show examples of ARM and highlighting the LinuxBuildImage is for x86-64.
/** Image "aws/codebuild/amazonlinux2-aarch64-standard:2.0". */
public static readonly AMAZON_LINUX_2_ARM_2 = LinuxArmBuildImage.codeBuildImage('aws/codebuild/amazonlinux2-aarch64-standard:2.0');

/**
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@skinny85 Any idea why the linter complains about @aws-cdk/aws-codebuild: error: [awslint:docs-public-apis:@aws-cdk/aws-codebuild.LinuxArmBuildImage.fromEcrRepository] Public API element must have a docstring?

If I'm not mistaken this should be it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Try putting the @returns at the end (where it should be, honestly 😛).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just copied what was already there in the file 🤷‍♂️. Seems to have fixed it though.

@tmmvn
Copy link
Contributor Author

tmmvn commented Mar 1, 2022

@skinny85 Passes tests but build fails with something that seems unrelated:

npm ERR! code ETARGET
npm ERR! notarget No matching version found for @aws-cdk/aws-iotevents-actions@1.144.0.
npm ERR! notarget In most cases you or one of your dependencies are requesting
npm ERR! notarget a package version that doesn't exist.

Want further actions from me or can this PR be considered ready?

@skinny85
Copy link
Contributor

skinny85 commented Mar 1, 2022

Try upgrading with the latest master - that should help.

@tmmvn tmmvn changed the title feat(codebuild): improve support for ARM build images feat(codebuild): improve support for arm build images Mar 1, 2022
@tmmvn
Copy link
Contributor Author

tmmvn commented Mar 1, 2022

@skinny85 That seems to have fixed that. Seems only validate-pr is missing any more. Not sure what that is?

Btw. I did a generic search for ARM in the GitHub issues and found ticket #9817, which I think is related. I probably did too specific of a search initially (or might have used aarch instead of ARM) so missed that one when creating my issue. I think this PR would solve that other ticket as well, though a bit differently (they were expecting a similar architecture setting / switching as I did).

@skinny85
Copy link
Contributor

skinny85 commented Mar 1, 2022

Yes, let's add #9817 to the list of issued fixed by this PR 🙂.

Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Looks great @codemeddler! Just a few minor notes on the code organization.

packages/@aws-cdk/aws-codebuild/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-codebuild/lib/project.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-codebuild/lib/project.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-codebuild/lib/project.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-codebuild/lib/project.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-codebuild/lib/project.ts Outdated Show resolved Hide resolved
@rix0rrr rix0rrr added feature-request A feature should be added or improved. p1 and removed p1 feature-request A feature should be added or improved. @aws-cdk/aws-codebuild Related to AWS CodeBuild labels Mar 4, 2022
Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Thanks for all your efforts on this @codemeddler! One last question.

@tmmvn
Copy link
Contributor Author

tmmvn commented Mar 5, 2022

Build was successful with the last discussion points as expected.

Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @codemeddler!

@mergify
Copy link
Contributor

mergify bot commented Mar 7, 2022

Thank you for contributing! Your pull request will be updated from master 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: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: ef4fc87
  • 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 4eac4de into aws:master Mar 7, 2022
@mergify
Copy link
Contributor

mergify bot commented Mar 7, 2022

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

TheRealAmazonKendra pushed a commit to TheRealAmazonKendra/aws-cdk that referenced this pull request Mar 11, 2022
Fixes aws#18916
Fixes aws#9817

### Motivation 
CDK currently has poor and hidden support for using ARM build images for CodeBuild that do not match what you can do with the Console. Currently, CDK has under LinuxBuildImage two constants not mentioned in the documentation. The constants internally map to a hidden ArmBuildImage class, which provides support for the standard CodeBuild ARM build images. That is the extent of the support, making ARM a second class citizen compared to x86-64 Linux and Windows build images as, for example, you can't use custom aarch64 ECR images.

### Changes
This pull request addresses the missing support by:
- renaming the previously hidden class ArmBuildImage to LinuxArmBuildImage (in case there are Windows ARM Build Images in the future).
- exporting LinuxArmBuildImage so it can be used.
- adding the two ARM constants present in LinuxBuildImage also to LinuxArmBuildImage. The constants are also left under LinuxBuildImage to not break backwards compatibility.
- adding the method fromEcrRepository() to support custom ARM build images.
- making the LinuxArmBuildImage closer to the LinuxBuildImage and WindowsBuildImage (built with props instead of just image name).
- updating documentation to show examples of ARM and highlighting the LinuxBuildImage is for x86-64.

### Testing
The unit test for ARM image is still valid.
----

*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
feature-request A feature should be added or improved. p1
Projects
None yet
4 participants