feat(aws-codebuild): allow using docker image assets as build images #1233
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change enables using Docker assets as CodeBuild images. In order to enable that, a few changes were required in how Docker assets are modeled: Extracted a low-level `DockerImageAsset` class into a new module called @aws-cdk/assets-docker. This module provides the basic interaction with the toolkit and allows higher level APIs such as ECR or CodeBuild to manage permissions on the asset repository. Since CodeBuild needs permissions at the resource policy level (since images are not pulled via an assumed role), the adopted repository custom resource was extended to support specifying an arbitrary policy document for the repository. This is made available via an overload of `addToResourcePolicy` implemented by `AdoptedRepository`. In order to fix #1232 and simplify, the protocol between the toolkit and the asset class have been changed to only pass in the _repository name_ and the _tag_ (together). Previously the ARN of the repository was used, but it is impossible to parse the repository name from the ARN using CFN's split/select. It is possible to produce the ARN from a name, and the repository is ensured to be "local" to the account/region. Modernized ECR import/export and converted `RepositoryRef` to `IRepository`, and modified `ImportedRepository` to only parse the repository name from ARN in case the ARN is concrete (not a token). Otherwise, the name is also required. Renamed `grantUseImage` to `grantPull` and added `grantPullPush`. Added `repositoryUriForTag(tag)`. Fixes #1219 BREAKING CHANGE: `ecr.RepositoryRef` has been replaced by `ecr.IRepository`, which means that `RepositoryRef.import` is now `Repository.import`. Futhermore, the CDK Toolkit must also be upgraded since the docker asset protocol was modified. `IRepository.grantUseImage` was renamed to `IRepository.grantPull`.
rix0rrr
reviewed
Nov 22, 2018
|
Integration test run: |
- Bump cxapi version to 0.19.0 (since protocol changed) - In integration tests, use local modules instead of "npm install" - Remove init template tests. Since they use "npm install" they can't really be validated like this against the local bits. We have integration tests for init template in our pipeline that work against the latest build.
skinny85
approved these changes
Nov 26, 2018
| * The ARN of the repository to import. | ||
| * | ||
| * If you only have a repository name and the repository is in the same account/region | ||
| * as the current stack, you can use the static method `Repository.arnForLocalRepository(name)` |
There was a problem hiding this comment.
Can't we just make ARN optional then, and default it to Repository.arnForLocalRepository(name) if it wasn't provided (of course, one of ARN-name would be required)?
| return { | ||
| repositoryArn: new cdk.Output(this, 'RepositoryArn', { value: this.repositoryArn }).makeImportValue().toString(), | ||
| }; | ||
| public get repositoryUri() { |
There was a problem hiding this comment.
Do we need both this method, and repositoryUriForTag?
There was a problem hiding this comment.
I think repo.repositoryUri is useful on it's own.
There was a problem hiding this comment.
Meh. I would just have repositoryUri take an optional tag argument and call it a day, but do as you wish.
added 10 commits
November 28, 2018 05:56
|
Thanks so much for taking the time to contribute to the AWS CDK ❤️ We will shortly assign someone to review this pull request and help get it
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change enables using Docker assets as CodeBuild images.
In order to enable that, a few changes were required in how Docker assets
are modeled:
Extracted a low-level
DockerImageAssetclass into a new module called@aws-cdk/assets-docker. This module provides the basic interaction with
the toolkit and allows higher level APIs such as ECR or CodeBuild to
manage permissions on the asset repository.
Since CodeBuild needs permissions at the resource policy level (since
images are not pulled via an assumed role), the adopted repository
custom resource was extended to support specifying an arbitrary policy
document for the repository. This is made available via an overload of
addToResourcePolicyimplemented byAdoptedRepository.In order to fix #1232 and simplify, the protocol between the toolkit and
the asset class have been changed to only pass in the repository name
and the tag (together). Previously the ARN of the repository was used,
but it is impossible to parse the repository name from the ARN using
CFN's split/select. It is possible to produce the ARN from a name, and
the repository is ensured to be "local" to the account/region.
Modernized ECR import/export and converted
RepositoryReftoIRepository,and modified
ImportedRepositoryto only parse the repository name fromARN in case the ARN is concrete (not a token). Otherwise, the name is
also required.
Renamed
grantUseImagetograntPulland addedgrantPullPush.Added
repositoryUriForTag(tag).Fixes #1219
BREAKING CHANGE:
ecr.RepositoryRefhas been replaced byecr.IRepository, whichmeans that
RepositoryRef.importis nowRepository.import. Futhermore, the CDKToolkit must also be upgraded since the docker asset protocol was modified.
IRepository.grantUseImagewas renamed toIRepository.grantPull.Pull Request Checklist
Please check all boxes (including N/A items)
Testing
tests
manually executed (paste output to the PR description)
(currently maintained in a private repo).
Documentation
Title and description
fix(module): <title>bug fix (patch)feat(module): <title>feature/capability (minor)chore(module): <title>won't appear in changelogbuild(module): <title>won't appear in changelogBREAKING CHANGE: <describe exactly what changed and how to achieve similar behavior + link to documentation/gist/issue if more details are required>Fixes #xxxorCloses #xxxBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.