-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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(AWS Lambda): Ensure function update works when image used #8786
fix(AWS Lambda): Ensure function update works when image used #8786
Conversation
9586c88
to
8f4e46b
Compare
Codecov Report
@@ Coverage Diff @@
## master #8786 +/- ##
==========================================
+ Coverage 87.74% 87.76% +0.02%
==========================================
Files 264 264
Lines 9780 9822 +42
==========================================
+ Hits 8581 8620 +39
- Misses 1199 1202 +3
Continue to review full report at Codecov.
|
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.
@pgrzesik looks great! I have just few minor suggestions
lib/plugins/aws/deployFunction.js
Outdated
if (didOneMinutePass) { | ||
throw new ServerlessError( | ||
'Retry timed out. Please try to deploy your function once again.' | ||
); | ||
} |
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 think this belongs within below if
condition (otherwise we risk hiding an error that anyway was supposed to be exposed)
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.
good call 👍
lib/plugins/aws/deployFunction.js
Outdated
'description' in functionObj && | ||
!_.isObject(functionObj.description) && |
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 see it reflects the original logic, but as we're updating it, I would improve it:
- In case of string property more natural to check for existence with
functionObj.description
- By schema this value can't be an object, so I would skip that check
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.
Great call, I believe I've applied it whenever it made sense and dropped checks for isObject
in places where schema prevents it
lib/plugins/aws/deployFunction.js
Outdated
'handler' in functionObj && | ||
!_.isObject(functionObj.handler) && |
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.
Same here
expect(stdoutData).to.include('Successfully updated function'); | ||
}); | ||
|
||
it('should cast non-string env variables to strings during update', async () => { |
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.
Let's skip this test. String coercion is done at configuration validation step. This functionality in all cases receives property values coerced to defined types
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 👍
In relation to discussion started in #8778.
This PR adds:
updateFunctionConfiguration
fails due to another update in progressupdateFunctionCode
call when image did not changeupdateFunctionConfiguration
when nothing changed/there's nothing to update