Repository navigation
fix(lib): reuse a bundled asset's artifact when its hash is unchanged #609
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
e0c4266
d31763d
cc63d9e
6b5ac3a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,7 @@ import { | |
| } from "./assets"; | ||
| import { AssetStaging } from "./asset-staging"; | ||
| import { IIgnoreStrategy } from "./ignore-strategy"; | ||
| import { findFileAboveCwd } from "./private/fs"; | ||
| import { findFileAboveCwd, renameSyncWithRetry } from "./private/fs"; | ||
| import { ISynthesisSession } from "./synthesize"; | ||
| import { addCustomSynthesis } from "./synthesize/synthesizer"; | ||
| import { TerraformStack } from "./terraform-stack"; | ||
|
|
@@ -74,6 +74,12 @@ export interface TerraformAssetConfig { | |
| * Extra information to fold into the hash (e.g. build instructions and | ||
| * other inputs). | ||
| * | ||
| * A bundled asset reuses the artifact an earlier synth staged for the same | ||
| * hash, so set this for build inputs the hash cannot see: files outside | ||
| * `path`, a mutable Docker tag such as `node:20`, or the options of a | ||
| * bundler without a `bundlerKey`. It is ignored when `assetHash` is set; | ||
| * change `assetHash` instead. | ||
| * | ||
| * @default - no extra hash | ||
| */ | ||
| readonly extraHash?: string; | ||
|
|
@@ -87,6 +93,10 @@ export interface TerraformAssetConfig { | |
| * The bundler's output shape must match `type`. See | ||
| * `AssetStagingOptions.bundler`. | ||
| * | ||
| * Under `SOURCE` hashing or a fixed `assetHash`, a synth keeps the artifact | ||
| * already staged for the same hash and `type`. See `extraHash` for inputs | ||
| * the hash cannot see; deleting the output directory also forces a rebuild. | ||
| * | ||
| * @default - the source is staged verbatim, with no build step | ||
| */ | ||
| readonly bundler?: IAssetBundler; | ||
|
|
@@ -136,6 +146,8 @@ export class TerraformAsset extends Construct implements IAsset { | |
| private readonly staging: AssetStaging; | ||
| // set when a bundler builds the artifact; names a single-file result | ||
| private readonly bundler?: IAssetBundler; | ||
| // true when the hash is known before the build, so a staged artifact can be reused | ||
| private readonly reusesStagedArtifact: boolean; | ||
|
|
||
| /** | ||
| * A Terraform Asset takes a file or directory outside of the CDK Terrain context and moves it into it. | ||
|
|
@@ -177,6 +189,8 @@ export class TerraformAsset extends Construct implements IAsset { | |
| } | ||
|
|
||
| this.bundler = config.bundler; | ||
| this.reusesStagedArtifact = | ||
| !!config.bundler && config.assetHashType !== AssetHashType.OUTPUT; | ||
|
|
||
| const outputFileName = config.bundler?.outputFileName; | ||
| if (outputFileName !== undefined) { | ||
|
|
@@ -270,21 +284,68 @@ export class TerraformAsset extends Construct implements IAsset { | |
| "..", | ||
| ); | ||
|
|
||
| // Cleanup existing assets | ||
| const previousVersionsFolder = path.join(basePath, this.namedFolder); | ||
| if (fs.existsSync(previousVersionsFolder)) { | ||
| fs.rmSync(previousVersionsFolder, { recursive: true }); | ||
| // Cleanup assets staged for previous hashes | ||
| const namedFolder = path.join(basePath, this.namedFolder); | ||
| const hashFolder = path.join(namedFolder, this.assetHash); | ||
| // Records how the artifact in the hash folder was packaged, which the | ||
| // hash does not cover: ARCHIVE and DIRECTORY share a hash. | ||
| const packagingFile = `${hashFolder}.json`; | ||
| if (fs.existsSync(namedFolder)) { | ||
| for (const entry of fs.readdirSync(namedFolder)) { | ||
| if (entry !== this.assetHash && entry !== `${this.assetHash}.json`) { | ||
| fs.rmSync(path.join(namedFolder, entry), { | ||
| recursive: true, | ||
| force: true, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // SOURCE and CUSTOM hashes are known before the build, so an artifact | ||
| // already staged for this hash and packaging is what the bundler would | ||
| // build again. | ||
| const targetPath = path.join(basePath, this.path); | ||
| const packaging = this.packaging; | ||
| const stagedPackaging = JSON.stringify({ | ||
| type: AssetType[this.type], | ||
| fileName: this.fileName, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Directory asset sidecar includes
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right. For a directory the file name was just the source's basename and isn't part of |
||
| }); | ||
| if ( | ||
| this.reusesStagedArtifact && | ||
| fs.existsSync(targetPath) && | ||
| fs.existsSync(packagingFile) && | ||
| fs.readFileSync(packagingFile, "utf8") === stagedPackaging | ||
| ) { | ||
| return; | ||
| } | ||
| fs.rmSync(packagingFile, { force: true }); | ||
|
|
||
| if (packaging.producesDirectory) { | ||
| fs.mkdirSync(targetPath, { recursive: true }); | ||
| } else { | ||
| fs.mkdirSync(path.dirname(targetPath), { recursive: true }); | ||
| if (!this.reusesStagedArtifact) { | ||
| fs.rmSync(hashFolder, { recursive: true, force: true }); | ||
| if (this.packaging.producesDirectory) { | ||
| fs.mkdirSync(targetPath, { recursive: true }); | ||
| } else { | ||
| fs.mkdirSync(path.dirname(targetPath), { recursive: true }); | ||
| } | ||
| this.staging.stage(targetPath); | ||
| return; | ||
| } | ||
|
|
||
| this.staging.stage(targetPath); | ||
| // Stage next to the hash folder and move it into place only once staging | ||
| // succeeded, so a failed or interrupted build never leaves a folder that | ||
| // a later synth would reuse. | ||
| const stagingFolder = `${hashFolder}.staging`; | ||
| fs.mkdirSync(stagingFolder, { recursive: true }); | ||
| try { | ||
| this.staging.stage( | ||
| this.packaging.producesDirectory | ||
| ? stagingFolder | ||
| : path.join(stagingFolder, this.fileName), | ||
| ); | ||
| fs.rmSync(hashFolder, { recursive: true, force: true }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing retry on cleanup
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 6b5ac3a. There's now an |
||
| renameSyncWithRetry(stagingFolder, hashFolder); | ||
| } finally { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. The |
||
| fs.rmSync(stagingFolder, { recursive: true, force: true }); | ||
| } | ||
| fs.writeFileSync(packagingFile, stagedPackaging); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stale artifact reuse — bundler swaps without
bundlerKeysilently serve old build outputThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the trade-off jsteinich and I went through in the first review. The hash is taken at construction, and the only thing that identifies a bundler in it is its
bundlerKey. Without one there's nothing stable to tell two bundlers apart: an object literal or a minified class has no reliable name. #582 asks for exactly this reuse (its repro bundler has no key), AWS CDK does the same, andIAssetBundler.bundlerKeyalready says an incomplete key reuses a stale artifact.LocalBundler,DockerBundlerandChainBundlerall set one. Turning reuse off for keyless bundlers would bring back the rebuild-every-synth behaviour for the case in the issue, so I left the code as is. I did add the choice of bundler to the inputs theextraHashdocs list, and the description calls this out as a behaviour change, withextraHashor deletingcdktf.outas the fix. Happy to revisit if you see it differently.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds good.