feat(aws-cdk): allow uploading Docker images as assets #982
Conversation
The construct library that takes advantage of this feature is in a separate branch, but this change already adds the ability for the toolkit to upload images. The code is integration tested in the other branch as well.
| tagParameter: string; | ||
| } | ||
|
|
||
| export type AssetMetadataEntry = FileAssetMetadataEntry | ContainerImageAssetMetadataEntry; |
There was a problem hiding this comment.
How's jsii dealing with this? Poorly, I think :(
There was a problem hiding this comment.
Good point. Doesn't actually matter as no user ever needs to call this (I hope).
@eladb, why don't we bundle @aws-cdk/cx-api as part of @aws-cdk/cdk?
There was a problem hiding this comment.
Because it's an implementation detail that there's a shared package with definitions between aws-cdk and @aws-cdk/cdk. Nobody else should need to see or interact with it. Therefore it does not need to be exposed over jsii.
As a practical matter, not exposing over jsii means we can safely use TypeScript-isms that otherwise wouldn't be allowed.
| selection: { | ||
| tagStatus: 'any', | ||
| countType: 'imageCountMoreThan', | ||
| countNumber: 5, |
There was a problem hiding this comment.
That seems excessively low to me. Especially since it is not user-customizable.
There was a problem hiding this comment.
Yes, it should be like 3 months or something
There was a problem hiding this comment.
In what scenario will those old images be used though? Every repository is tied to a single stack in a single region, and once the stack is updated all references will point to the new image, which immediately follows deployment.
I can see retaining two images for quick rollback purposes, and I put a little more margin on there "just because" that feels safe.
But 3 months? Don't forget that these images cost money as well. We already have a user starting to complain about the size of their asset bucket.
There was a problem hiding this comment.
Keep in mind, we can't tell if these images are currently deployed somewhere. So if 5 builds happen that fail testing and never get deployed, my production image will be deleted. Time-based policy means I'm banking on deploying at least one a quarter. I would actually say 6 months is safer...
| const ecr = await this.props.sdk.ecr(this.props.environment, Mode.ForWriting); | ||
|
|
||
| // Create the repository if it doesn't exist yet | ||
| const repositoryName = 'cdk/' + id.replace(/[:/]/g, '-').toLowerCase(); |
There was a problem hiding this comment.
Does it really make sense to use a cdk/ prefix? My intuition is that the repository name should be something the user controls, no?
There was a problem hiding this comment.
I'm treating this akin to the Asset bucket. Why should the user not be allowed to care about the asset bucket name, but be allowed to care about repository names?
| selection: { | ||
| tagStatus: 'any', | ||
| countType: 'imageCountMoreThan', | ||
| countNumber: 5, |
There was a problem hiding this comment.
Yes, it should be like 3 months or something
|
|
||
| const command = ['docker', | ||
| 'build', | ||
| '--quiet', |
There was a problem hiding this comment.
Do not pass --quiet if --verbose is used
There was a problem hiding this comment.
I am not 100% sure we don't want to show this output. It can take a looong time
There was a problem hiding this comment.
--quiet is the way to make Docker output machine-friendly output (image ID of the just-produced image).
Without quiet, it prints human-centric garble that contains strings that we need to parse the image ID out of, and no idea how stable that is.
(If you're going to look: there are some options for this in the Docker manual but those only seem to apply to quite recent versions. At least they don't work on mine. I was aiming for compatibility here.)
My original implementation did have the implementation you proposed, but Docker CLI makes it hard.
There was a problem hiding this comment.
does it print it to STDOUT or STDERR?
Literally in the PR message:
I can't include these other things because they depend on a bunch of code that still needs to be merged. The merge will (hopefully) be forthcoming so if you don't want to approve this just yet we might be able to wait without delaying things for too long. |
| * spurious rebuilds. However, this digest is calculated over a manifest that | ||
| * includes metadata that is liable to change. For example, as soon as we | ||
| * push the Docker image to a repository, the digest changes. This makes the | ||
| * digest worthless to determe whether we already pushed an image, for example. |
There was a problem hiding this comment.
I'm confused by this paragraph. You are correct that the image digest can change on image push, but where do you use the fingerprint value to determine whether it has already been pushed? The registry will do that for you per-layer.
|
Subsumed by ECS PR |
The construct library that takes advantage of this feature is in a
separate branch, but this change already adds the ability for the
toolkit to upload images.
The code is integration tested in the other branch as well.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.