feat(assets): Add deploy-time content hash #2334
Conversation
Introduces an `IAsset` interface that centralizes common aspects about assets, such as the `sourceHash` and `bundleHash` properties. The `sourceHash` fingerprints the objects that are used as the source for the asset bundling logic, and is available at synthesis time (it can for example be injected in construct IDs when it one wants to ensure a new logical ID is issued for every new version of the asset). The `bundleHash` fingerprints the result of the bundling logic, and is more accurate than `sourceHash` (in that, if the same source can produce different artifacts at different points in time, the `sourceHash` will remain un-changed, but the `bundleHash` will change. The `bundleHash` is however a deploy-time value and thus cannot be used in construct IDs. Fixes #1400
RomainMuller
requested review from
skinny85,
SoManyHs and
a team
as code owners
packages/@aws-cdk/assets-docker/lib/adopt-repository/handler.js
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/assets-docker/lib/adopt-repository/handler.js
Outdated
Show resolved
Hide resolved
| } | ||
| const rootDirectory = fs.statSync(fileOrDirectory).isDirectory() | ||
| ? fileOrDirectory | ||
| : path.dirname(fileOrDirectory); |
There was a problem hiding this comment.
missing tests for all the new options
There was a problem hiding this comment.
As far as I know, I have not introduced any new options 😅 but yes, some of that surface is untested. I'll se if I can get around to fixing that.
There was a problem hiding this comment.
The ability to exclude files has been added (previously these options were ignored)
| @@ -692,13 +692,13 @@ | |||
| "RepositoryName" | |||
| ] | |||
| }, | |||
| ":", | |||
| "@sha256:", | |||
There was a problem hiding this comment.
The mix of Fn::Select and Fn::Join in the Image property looks weird to me (+ it ends up with amazonaws.com in the final string and not AWS::URLSuffix), shouldn't this result in something like:
{
"Image": {
"Fn::Join": [
"",
[
{ "Ref": "AWS::AccountId" },
".dkr.ecr.",
{ "Ref": "AWS::Region" },
".",
{ "Ref": "AWS::URLSuffix" },
"/",
{ "Fn::GetAtt": [ "EventImageAdoptRepositoryDFAAC242", "RepositoryName"] },
"@sha256:"
{ "Ref": "EventImageImageNameE972A8B1" },
]
]
}
}or { "Ref": "AdoptEcrRepositorydbc60defc59544bcaa5c28c95d68f62cCodeArtifactHash8BCBAA49" } for the last element
There was a problem hiding this comment.
+after pushing, now that the repo digest is used we have the full "resolved URI" (with region, accountid, etc.), is it still needed to use Refs in this case? A single Ref to the parameter should be possible?
There was a problem hiding this comment.
We're passing the repository name, not full URI, in a parameter. This is because the CustomResource that "adopts" the CDK-toolkit-created ECR repository works on the name (for that is the ECR API). I'm torn myself on whether I should cut the chase and just pass the URI around, but it makes parsing the name out of the URI an awkward process (I'd rather recompose the URI than parse it out).
RomainMuller
force-pushed
the
rmuller/asset-content-hash
branch
from
729f535
to
769f2a3
Compare
| } | ||
| const rootDirectory = fs.statSync(fileOrDirectory).isDirectory() | ||
| ? fileOrDirectory | ||
| : path.dirname(fileOrDirectory); |
There was a problem hiding this comment.
The ability to exclude files has been added (previously these options were ignored)
| @@ -53,6 +58,7 @@ export class Staging extends Construct { | |||
| super(scope, id); | |||
|
|
|||
| this.sourcePath = props.sourcePath; | |||
| this.sourceHash = fingerprint(this.sourcePath); | |||
There was a problem hiding this comment.
I think we should expose the copy options (exclude, symlinks), but this can be in a subsequent PR.
Introduces an
IAssetinterface that centralizes common aspects aboutassets, such as the
sourceHashandbundleHashproperties.The
sourceHashfingerprints the objects that are used as the sourcefor the asset bundling logic, and is available at synthesis time (it can
for example be injected in construct IDs when it one wants to ensure a
new logical ID is issued for every new version of the asset).
The
bundleHashfingerprints the result of the bundling logic, and ismore accurate than
sourceHash(in that, if the same source can producedifferent artifacts at different points in time, the
sourceHashwillremain un-changed, but the
bundleHashwill change. ThebundleHashishowever a deploy-time value and thus cannot be used in construct IDs.
Fixes #1400
Pull Request Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.