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): add functionality to allow using private registry an… #2796

Closed
wants to merge 4 commits into from

Conversation

Kaixiang-AWS
Copy link
Contributor

…d cross-account ECR repository as build image

Fixes #2175


Pull Request Checklist

  • Testing
    • Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
    • CLI change?: coordinate update of integration tests with team
    • cdk-init template change?: coordinated update of integration tests with team
  • Docs
    • jsdocs: All public APIs documented
    • README: README and/or documentation topic updated
    • Design: For significant features, design document added to design folder
  • Title and Description
    • Change type: title prefixed with fix, feat and module name in parens, which will appear in changelog
    • Title: use lower-case and doesn't end with a period
    • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
    • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
  • Sensitive Modules (requires 2 PR approvers)
    • IAM Policy Document (in @aws-cdk/aws-iam)
    • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
    • Grant APIs (only if not based on official documentation with a reference)

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

…d cross-account ECR repository as build image

Fixes aws#2175
@Kaixiang-AWS Kaixiang-AWS requested review from RomainMuller, skinny85 and a team as code owners June 7, 2019 23:33
'ecr:BatchCheckLayerAvailability'
);
function isECRImage(imageUri: string) {
return /^(.+).dkr.ecr.(.+).amazonaws.com[.]{0,1}[a-z]{0,3}\/([^:]+):?.*$/.test(imageUri);
Copy link
Contributor

Choose a reason for hiding this comment

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

You should ensure imageUri can be parsed using Token.unresolved

@@ -1023,7 +1060,7 @@ export interface IBuildImage {
*
* You can also specify a custom image using one of the static methods:
*
* - LinuxBuildImage.fromDockerHub(image)
* - LinuxBuildImage.fromDockerRegistry(image[, secretsManagerCredential])
Copy link
Contributor

Choose a reason for hiding this comment

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

@skinny85 @SoManyHs @rix0rrr do you think it makes sense to merge this API with the one we have for ECS?

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, we'd probably need some generic modeling of Docker images, using @aws-cdk/docker or something.

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed offline. We decided not create a general Docker image API. Given that, I believe this code is fine - what do you think @eladb ?

@skinny85
Copy link
Contributor

Hey @Kaixiang-AWS ,

apologies for not helping you with this PR sooner. I promise I'll improve from now on 🤞. I see there are some conflicts with master, and I believe some accidental changes in the docker-assets package that should not be there currently in the diff. Would you mind merging/rebasing (whichever you prefer) with the newest tip of the master branch, and we'll pick it up from there?

Thanks,
Adam

@Kaixiang-AWS
Copy link
Contributor Author

Hey @Kaixiang-AWS ,

apologies for not helping you with this PR sooner. I promise I'll improve from now on 🤞. I see there are some conflicts with master, and I believe some accidental changes in the docker-assets package that should not be there currently in the diff. Would you mind merging/rebasing (whichever you prefer) with the newest tip of the master branch, and we'll pick it up from there?

Thanks,
Adam

I'm working on the rebasing and will have a PR out soon

@skinny85
Copy link
Contributor

@Kaixiang-AWS ,

can we close this PR, now that #3049 has been opened?

@Kaixiang-AWS
Copy link
Contributor Author

tracking in #3049

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CodeBuild project environment should support ImagePullCredentialsType
4 participants