-
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
feat(cli): hotswap deployments #15748
Conversation
@eladb thanks for the review! Incorporated all of your comments, would appreciate another pass! |
deploy
command
@eladb thanks for the review! Incorporated all of your comments, would appreciate another pass. |
@BenChaimberg thanks a lot for the review! I've pushed a new revision addressing your comments, would appreciate another pass! |
return hotswapDeploymentResult; | ||
} | ||
// could not short-circuit the deployment, perform a full CFN deploy instead | ||
print('Could not perform a hotswap deployment, as the stack %s contains non-Asset changes', stackArtifact.displayName); |
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.
💅 Putting this here is not extensible. The tryHotswapDeployment
function should probably return a reason saying why it couldn't do it.
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.
Can you elaborate what would you like to see here? Should the Lambda "hotswap detector" return something like "Change is to property 'X', while only changes to the 'Code' property are supported", or something like that? Or did you mean something else?
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, I meant something like that.
const resolvedEnv = await sdkProvider.resolveEnvironment(stackArtifact.environment); | ||
const hotswappableChanges = findAllHotswappableChanges(stackChanges, { | ||
...assetParams, | ||
'AWS::Region': resolvedEnv.region, |
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.
AWS::Partition
as well probably
And I'd think AWS::URLSuffix
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.
Sure, but where can we get those from? I'd rather avoid doing more service calls here (this is supposed to be as fast as possible).
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.
cdk-assets
has code to get these values.
@rix0rrr @BenChaimberg thanks for the review, the PR should be ready for another pass! |
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.
lgtm
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.
@calvin-cc In general, looks great! 2 questions.
let disableRollback; | ||
|
||
if (options.rollback === undefined && options.hotswap === true) { | ||
disableRollback = { DisableRollback: true }; | ||
} else { | ||
disableRollback = options.rollback === false ? { DisableRollback: true } : undefined; | ||
} |
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.
We're repeating the { DisableRollback: true }
expression for no reason here.
let disableRollback; | |
if (options.rollback === undefined && options.hotswap === true) { | |
disableRollback = { DisableRollback: true }; | |
} else { | |
disableRollback = options.rollback === false ? { DisableRollback: true } : undefined; | |
} | |
const shouldDisableRollback = (options.rollback === undefined && options.hotswap === true) || options.rollback === false; | |
const disableRollback = shouldDisableRollback ? { DisableRollback: true } : undefined; |
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.
Looks great! One more small simplification.
packages/aws-cdk/lib/cdk-toolkit.ts
Outdated
@@ -196,7 +203,8 @@ export class CdkToolkit { | |||
usePreviousParameters: options.usePreviousParameters, | |||
progress: options.progress, | |||
ci: options.ci, | |||
rollback: options.rollback, | |||
rollback: shouldDisableRollback ? false : true, |
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.
Since you moved this here, we can simplify further:
rollback: shouldDisableRollback ? false : true, | |
rollback: options.rollback === undefined && options.hotswap ? false : options.rollback, |
And get rid of the shouldDisableRollback
variable.
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.
We're very close! Just revert the readonly
change, and move the logic down a little bit.
@@ -181,7 +185,16 @@ export interface DeployStackOptions { | |||
* | |||
* @default true | |||
*/ | |||
readonly rollback?: boolean; | |||
rollback?: boolean; |
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.
I don't like changing this to not be readonly
. See my solution below on how to handle this in a more idiomatic way. This change should be reverted.
const shouldDisableRollback = (options.rollback === undefined && options.hotswap === true) || options.rollback === false; | ||
options.rollback = shouldDisableRollback ? false : true; |
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.
Not a fan of doing this here. tryHotswapDeployment()
doesn't use this option, so dealing with it here is a little confusing to me. Let's move this down to prepareAndExecuteChangeSet()
, where it's actually used.
@@ -250,6 +286,7 @@ export async function deployStack(options: DeployStackOptions): Promise<DeploySt | |||
|
|||
debug(`Attempting to create ChangeSet with name ${changeSetName} to ${update ? 'update' : 'create'} stack ${deployName}`); | |||
print('%s: creating CloudFormation changeset...', colors.bold(deployName)); | |||
const executionId = uuid.v4(); | |||
const changeSet = await cfn.createChangeSet({ | |||
StackName: deployName, | |||
ChangeSetName: changeSetName, |
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.
The defaulting of rollback
should be done here. I can't comment on the line, because it wasn't changed, but it should be done on line 332.
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.
@comcalvi looks great! Thanks for the change. I will merge this PR in sec.
Mergify dismissed it accidentally
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
This is the first PR implementing the ["Accelerated personal deployments" RFC](https://github.com/aws/aws-cdk-rfcs/blob/master/text/0001-cdk-update.md). It adds a (boolean) `--hotswap` flag to the `deploy` command that attempts to perform a short-circuit deployment, updating the resource directly, and skipping CloudFormation. If we detect that the current change cannot be short-circuited (because it contains an infrastructure change to the CDK code, most likely), we fall back on performing a full CloudFormation deployment, same as if `cdk deploy` was called without the `--hotswap` flag. In this PR, the new switch supports only Lambda functions. Later PRs will add support for new resource types. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This is the first PR implementing the ["Accelerated personal deployments" RFC](https://github.com/aws/aws-cdk-rfcs/blob/master/text/0001-cdk-update.md). It adds a (boolean) `--hotswap` flag to the `deploy` command that attempts to perform a short-circuit deployment, updating the resource directly, and skipping CloudFormation. If we detect that the current change cannot be short-circuited (because it contains an infrastructure change to the CDK code, most likely), we fall back on performing a full CloudFormation deployment, same as if `cdk deploy` was called without the `--hotswap` flag. In this PR, the new switch supports only Lambda functions. Later PRs will add support for new resource types. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Thanks so much for the work on this! I'm eager to try it out but it doesn't seem to be functional yet for nested stacks. Are there plans to support that? |
@revmischa has opened #16508 to track this, so let's move the conversation there 🙂. |
This is the first PR implementing the "Accelerated personal deployments" RFC.
It adds a (boolean)
--hotswap
flag to thedeploy
command that attempts to perform a short-circuit deployment, updating the resource directly, and skipping CloudFormation.If we detect that the current change cannot be short-circuited
(because it contains an infrastructure change to the CDK code, most likely),
we fall back on performing a full CloudFormation deployment,
same as if
cdk deploy
was called without the--hotswap
flag.In this PR, the new switch supports only Lambda functions. Later PRs will add support for new resource types.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license