-
Notifications
You must be signed in to change notification settings - Fork 4k
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
fix(cli): build assets before deploying any stacks #21513
Changes from 7 commits
eb8f2b4
7da699f
25bdc39
0ce27cb
d7cbbee
ba02b1c
5ff973b
203ed6d
7afd1b6
2f10ed6
d82eb0a
611b370
f29d4b6
070d2e6
ba2d57e
fb9bf4d
9a18370
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 | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -236,6 +236,13 @@ export interface DeployStackOptions { | |||||||
* @default - Use the stored template | ||||||||
*/ | ||||||||
readonly overrideTemplate?: any; | ||||||||
|
||||||||
/** | ||||||||
* Disable asset publishing | ||||||||
* | ||||||||
* @default false | ||||||||
*/ | ||||||||
readonly disableAssetPublishing?: boolean; | ||||||||
} | ||||||||
|
||||||||
export interface DestroyStackOptions { | ||||||||
|
@@ -338,9 +345,14 @@ export class CloudFormationDeployments { | |||||||
|
||||||||
const toolkitInfo = await ToolkitInfo.lookup(resolvedEnvironment, stackSdk, options.toolkitStackName); | ||||||||
|
||||||||
// Publish any assets before doing the actual deploy (do not publish any assets on import operation) | ||||||||
if (options.resourcesToImport === undefined) { | ||||||||
await this.publishStackAssets(options.stack, toolkitInfo); | ||||||||
const disableAssetPublishing = options.disableAssetPublishing ?? false; | ||||||||
const isImporting = options.resourcesToImport !== undefined; | ||||||||
// Determine whether we should publish assets. We don't publish if assets | ||||||||
// are prepublished or during an import operation. | ||||||||
const shouldPublish = !disableAssetPublishing && !isImporting; | ||||||||
|
||||||||
if (shouldPublish) { | ||||||||
await this._publishStackAssets(options.stack, toolkitInfo); | ||||||||
} | ||||||||
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. All this is to maintain 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.
I'd want someone else to confirm this, but from what I can tell it's up for changes. |
||||||||
|
||||||||
// Do a verification of the bootstrap stack version | ||||||||
|
@@ -451,10 +463,20 @@ export class CloudFormationDeployments { | |||||||
}; | ||||||||
} | ||||||||
|
||||||||
/** | ||||||||
* Publish a stack's assets. | ||||||||
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.
Suggested change
|
||||||||
*/ | ||||||||
public async publishStackAssets(options: PublishStackAssetsOptions) { | ||||||||
const { stackSdk, resolvedEnvironment } = await this.prepareSdkFor(options.stack, options.roleArn); | ||||||||
const toolkitInfo = await ToolkitInfo.lookup(resolvedEnvironment, stackSdk, options.toolkitStackName); | ||||||||
|
||||||||
await this._publishStackAssets(options.stack, toolkitInfo); | ||||||||
} | ||||||||
|
||||||||
/** | ||||||||
* Publish all asset manifests that are referenced by the given stack | ||||||||
*/ | ||||||||
private async publishStackAssets(stack: cxapi.CloudFormationStackArtifact, toolkitInfo: ToolkitInfo) { | ||||||||
private async _publishStackAssets(stack: cxapi.CloudFormationStackArtifact, toolkitInfo: ToolkitInfo) { | ||||||||
const stackEnv = await this.sdkProvider.resolveEnvironment(stack.environment); | ||||||||
const assetArtifacts = stack.dependencies.filter(cxapi.AssetManifestArtifact.isAssetManifestArtifact); | ||||||||
|
||||||||
|
@@ -488,3 +510,27 @@ export class CloudFormationDeployments { | |||||||
} | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
/** | ||||||||
* Options for publishing stack assets. | ||||||||
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.
Suggested change
|
||||||||
*/ | ||||||||
export interface PublishStackAssetsOptions { | ||||||||
/** | ||||||||
* Stack to publish assets for | ||||||||
*/ | ||||||||
readonly stack: cxapi.CloudFormationStackArtifact; | ||||||||
|
||||||||
/** | ||||||||
* Execution role for publishing assets | ||||||||
* | ||||||||
* @default - Current role | ||||||||
*/ | ||||||||
readonly roleArn?: string; | ||||||||
|
||||||||
/** | ||||||||
* Name of the toolkit stack, if not the default name | ||||||||
* | ||||||||
* @default 'CDKToolkit' | ||||||||
*/ | ||||||||
readonly toolkitStackName?: string; | ||||||||
} | ||||||||
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.
Suggested change
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
import * as cxapi from '@aws-cdk/cx-api'; | ||
import PQueue from 'p-queue'; | ||
|
||
type Options = { | ||
concurrency: number; | ||
publishStackAssets: (stack: cxapi.CloudFormationStackArtifact) => Promise<void>; | ||
}; | ||
|
||
export const publishAllStackAssets = async (stacks: cxapi.CloudFormationStackArtifact[], options: Options): Promise<void> => { | ||
const { concurrency, publishStackAssets } = options; | ||
|
||
const queue = new PQueue({ concurrency }); | ||
const publishingErrors: Error[] = []; | ||
|
||
for (const stack of stacks) { | ||
queue.add(async () => { | ||
await publishStackAssets(stack); | ||
}).catch((err) => { | ||
publishingErrors.push(err); | ||
}); | ||
} | ||
|
||
await queue.onIdle(); | ||
|
||
if (publishingErrors.length) { | ||
throw Error(`Publishing Assets Failed: ${publishingErrors.join(', ')}`); | ||
} | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
import * as cxapi from '@aws-cdk/cx-api'; | ||
import { publishAllStackAssets } from '../lib/publish'; | ||
|
||
type Stack = cxapi.CloudFormationStackArtifact; | ||
|
||
describe('publishAllStackAssets', () => { | ||
const A = { id: 'A' }; | ||
const B = { id: 'B' }; | ||
const C = { id: 'C' }; | ||
const concurrency = 3; | ||
const toPublish = [A, B, C] as unknown as Stack[]; | ||
|
||
const sleep = async (duration: number) => new Promise<void>((resolve) => setTimeout(() => resolve(), duration)); | ||
|
||
test('publish', async () => { | ||
// GIVEN | ||
const publishStackAssets = jest.fn(() => sleep(1)); | ||
|
||
// WHEN/THEN | ||
await expect(publishAllStackAssets(toPublish, { concurrency, publishStackAssets })) | ||
.resolves | ||
.toBeUndefined(); | ||
|
||
expect(publishStackAssets).toBeCalledTimes(3); | ||
expect(publishStackAssets).toBeCalledWith(A); | ||
expect(publishStackAssets).toBeCalledWith(B); | ||
expect(publishStackAssets).toBeCalledWith(C); | ||
}); | ||
|
||
test('errors', async () => { | ||
// GIVEN | ||
const publishStackAssets = async () => { throw new Error('Message'); }; | ||
|
||
// WHEN/THEN | ||
await expect(publishAllStackAssets(toPublish, { concurrency, publishStackAssets })) | ||
.rejects | ||
.toThrow('Publishing Assets Failed: Error: Message, Error: Message, Error: Message'); | ||
}); | ||
}); |
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.
Thinking out loud here, would this be easier to understand if it were this?
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.
Yes please! Negative arguments break my brains.