feat(cli): support for hotswapping Lambda Versions and Aliases#18145
feat(cli): support for hotswapping Lambda Versions and Aliases#18145mergify[bot] merged 2 commits intoaws:masterfrom
Conversation
|
|
rix0rrr
left a comment
There was a problem hiding this comment.
I'm good with the functionality changes, but the logic could be refactored for readability.
| identicalRemovalChange[1].oldValue, | ||
| addChange.newValue, |
There was a problem hiding this comment.
Aren't oldValue and newValue going to be identical by definition here?
There was a problem hiding this comment.
Yes. I still feel like using the different values is clearer though (same in the lines below). What do you think?
There was a problem hiding this comment.
Maybe. Once you factor it out to a function I care less.
| addChange.newValue, | ||
| { | ||
| resourceType: { | ||
| oldType: identicalRemovalChange[1].oldResourceType, |
| // no hotswappable changes found, so at least one IRRELEVANT means we can ignore this change; | ||
| // otherwise, all answers are REQUIRES_FULL_DEPLOYMENT, so this means we can't hotswap this change, | ||
| // and have to do a full deployment instead | ||
| if (hotswapDetectionResults.every(hdr => hdr === ChangeHotswapImpact.REQUIRES_FULL_DEPLOYMENT)) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
Or, have every function not return a ChangeHotswapResult but a ChangeHotswapResult[], and return an empty array instead of IRRELEVANT.
There was a problem hiding this comment.
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.
a875be0 to
b6c96cd
Compare
|
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. |
b6c96cd to
ec5e484
Compare
There was a problem hiding this comment.
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] => IRRELEVANTThe 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.
That is actually not the current behavior - any
Yes, I'm pretty sure this is the correct behavior. |
|
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). |
…8145) 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 aws#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 aws#17043 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
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_DEPLOYMENTfor it;if at least one detector returns
IRRELEVANT, we ignore this change.This allows us to get rid of the awkward
EmptyHotswapOperationthat 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