feat(AWS Deploy): Ensure consistent function state in deploy function
#10288
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10288 +/- ##
==========================================
+ Coverage 85.39% 85.40% +0.01%
==========================================
Files 339 339
Lines 13854 13877 +23
==========================================
+ Hits 11830 11852 +22
- Misses 2024 2025 +1
Continue to review full report at Codecov.
|
pgrzesik
force-pushed
the
ensure-fn-state-in-deploy-function
branch
from
3373379
to
407cd6b
Compare
lib/plugins/aws/deployFunction.js
Outdated
| @@ -22,6 +22,8 @@ class AwsDeployFunction { | |||
| path.join(this.serverless.serviceDir || '.', '.serverless'); | |||
| this.provider = this.serverless.getProvider('aws'); | |||
|
|
|||
| this._shouldEnsureFunctionState = false; | |||
There was a problem hiding this comment.
This internal plugin is considered private in its own, so I think there's no need to additionally prefix internal properties with _
Also why do we need that property? If it's to differentiate between scenarios when there was no need for update, so no need to confirm on state, wouldn't it be better to update updateFunctionConfiguration so it returns true/false depending on whether update was made, and then follow up with state confirmation only when true is returned?
There was a problem hiding this comment.
I believe we can simplify it this way 👍
There was a problem hiding this comment.
Sorry, I've just noticed my push didn't go through - we need to record it both for updateFunctionCode and updateFunctionConfiguration because configuration update can be skipped and function might be still in bad state after only code update - I've changed the variable a little bit
| @@ -76,6 +78,9 @@ class AwsDeployFunction { | |||
| await this.deployFunction(); | |||
| } | |||
| await this.updateFunctionConfiguration(); | |||
| if (this._shouldEnsureFunctionState) { | |||
| await this.ensureFunctionState(); | |||
There was a problem hiding this comment.
I thought the issue on our side is that we need to confirm on function state after this.deployFunction() is done and before doing this.updateFunctionConfiguration() (as it may fail if it's not settled).
While all we do in this PR is simply prolonging command execution so it doesn't exit until the function state settles. If that's the case, then the only thing we're fixing is that following up with another command won't crash due to function being in interim state due to effect of previous command. Is that correct?
There was a problem hiding this comment.
I thought the same thing, but after finally being able to reproduce this issue, I've realized and noticed that in recent versions of the Framework, the retry for updateFunctionConfiguration is already handling that very well so there's no need to add any extra steps which will pretty much do the same thing.
While all we do in this PR is simply prolonging command execution so it doesn't exit until the function state settles. If that's the case, then the only thing we're fixing is that following up with another command won't crash due to function being in interim state due to effect of previous command. Is that correct?
Correct 👍
pgrzesik
force-pushed
the
ensure-fn-state-in-deploy-function
branch
from
407cd6b
to
5a59236
Compare
After doing a bit of more research I've noticed that at the moment,
updateFunctionConfigurationis already resilient and does retries in situations where function is still being updated, so I've decided to stick to only ensuring that function is in updated state at the end ofdeploy function.