feat(cli): support for hotswapping Lambda Versions and Aliases

[skinny85]

This extends the existing hotswapping support for Lambda Functions to also work for Versions and Aliases.

Implementation-wise, this required quite a bit of changes, as Versions are immutable in CloudFormation,
and the only way you can change them is by replacing them;
so, we had to add the notion of replacement changes to our hotswapping logic
(as, up to this point, they were simply a pair of "delete" and "add" changes,
which would result in hotswapping determining it can't proceed,
and falling back to a full CloudFormation deployment).

I also modified the main hotswapping algorithm:
now, a resource change is considered non-hotswappable if all detectors return REQUIRES_FULL_DEPLOYMENT for it;
if at least one detector returns IRRELEVANT, we ignore this change.
This allows us to get rid of the awkward EmptyHotswapOperation that we had to use before in these situations.

I also made a few small tweaks to the printing messages added in #18058:
I no longer prefix them with the name of the service,
as now hotswapping can affect different resource types, and it looked a bit awkward with that prefix present.

Fixes #17043

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[gitpod-io]

[rix0rrr]

I'm good with the functionality changes, but the logic could be refactored for readability.

[rix0rrr]

Aren't oldValue and newValue going to be identical by definition here?

[skinny85]

Yes. I still feel like using the different values is clearer though (same in the lines below). What do you think?

[rix0rrr]

Maybe. Once you factor it out to a function I care less.

[rix0rrr]

Same here

[skinny85]

Same as #18145 (comment).

[rix0rrr]

This condition might technically be correct, but it reads weird to me:

• Code says: "we only need to need to deploy if everything says we need to deploy"
• My expectation: "we need to deploy as soon as something says we need to deploy"

The code doesn't jive with my expectations, making me do a double- and triple-take here.

Isn't the following more in line with the intent of what you're trying to convey:

if (!hotswapDetectionResults.some(hdr => hdr === ChangeHotswapImpact.IRRELEVANT)) {

"we only need to deploy as long as nothing says the change is irrelevant"

I get that those statements are equivalent for the machine(*), but they're not equivalent to a human reading them.

(*) given the current possible enum values, but who knows what we might add in the future? We might want to be robust against future extensions

[rix0rrr]

By the way, an even more accurate modeling I think would be the following:

if (results.filter(hdr => hdr !== IRRELEVANT).some(hdr => hdr === REQUIRES_FULL_DEPLOYMENT)) {
  doFullDeployment();
}

"After taking out all the IRRELEVANTs, if something is still asking for a full deployment then let's do that"

[rix0rrr]

Or, have every function not return a ChangeHotswapResult but a ChangeHotswapResult[], and return an empty array instead of IRRELEVANT.

[skinny85]

Changed the condition to !hotswapDetectionResults.some(hdr => hdr === ChangeHotswapImpact.IRRELEVANT) - I actually debated between that, and the every() condition that is there in this version of the PR, so I like the change.

hotswapDetectionResults.filter(hdr => hdr !== IRRELEVANT).some(hdr => hdr === REQUIRES_FULL_DEPLOYMENT) actually means something different - "if there's at least one REQUIRES_FULL_DEPLOYMENT, we can't hotswap", which is the current logic, which this PR wants to change. So, it would have to be !hotswapDetectionResults.filter(hdr => hdr !== REQUIRES_FULL_DEPLOYMENT).some(hdr => hdr === IRRELEVANT), which I don't think reads that well.

[skinny85]

Thanks for the great review @rix0rrr. I agree with all of your points. Incorporated all of your changes, let me know if the code looks better now.

[rix0rrr]

Looks good. I'm still not 100% sure on the change of the meaning of IRRELEVANT. The new behavior seems counterintuitive when multiple hotswappers compete for the same resource... but maybe that simply never happens, and is never going to happen.

Also, it seems that IRRELEVANT is a replacement for EmptyHotSwapOperation. Are we sure that the semantics are the same?

As in, it seems to me that:

// Original
[new EmptyHotSwapOperation(), REQUIRES_FULL_DEPLOMENT] => REQUIRES_FULL_DEPLOYMENT

// { replace new EmptyHotSwapOperation() => IRRELEVANT }

// New
[IRRELEVANT, REQUIRES_FULL_DEPLOMENT] => IRRELEVANT

The behavior would have changed? But maybe I just don't understand it correctly.

In any case, if you feel comfortable with the change, feel free to merge.

[skinny85]

Also, it seems that IRRELEVANT is a replacement for EmptyHotSwapOperation. Are we sure that the semantics are the same?

As in, it seems to me that:

// Original
[new EmptyHotSwapOperation(), REQUIRES_FULL_DEPLOMENT] => REQUIRES_FULL_DEPLOYMENT

// { replace new EmptyHotSwapOperation() => IRRELEVANT }

// New
[IRRELEVANT, REQUIRES_FULL_DEPLOMENT] => IRRELEVANT

The behavior would have changed? But maybe I just don't understand it correctly.

That is actually not the current behavior - any HotswapOperation overrides REQUIRES_FULL_DEPLOYMENT, so this would result in the EmptyHotswapOperation getting executed.

Looks good. I'm still not 100% sure on the change of the meaning of IRRELEVANT. The new behavior seems counterintuitive when multiple hotswappers compete for the same resource... but maybe that simply never happens, and is never going to happen.

In any case, if you feel comfortable with the change, feel free to merge.

Yes, I'm pretty sure this is the correct behavior. EmptyHotswapOperation is kind of a "stopgap" that doesn't really work correctly (for example, it makes the deployment think it did some changes, where none actually happened, which is confusing). I think the version of the hotswap algorithm in this PR is the correct one.

[mergify]

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-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
• Commit ID: 3f2fe3a
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[mergify]

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).