feat: Support provider.vpcEgress configuration
#271
Conversation
fcr1193
changed the title
|
Hi @pgrzesik, do you think that we can move this PR? This feature will be helpful. |
There was a problem hiding this comment.
Thanks @fcr1193 - I've checked the changes and it looks okay, but I don't think it's working as intended. Additionally, it would be great to cover proposed changes by tests (see the original PR) so we're sure that they're working correctly and do not break existing functionality
package/lib/compileFunctions.js
Outdated
| @@ -57,6 +58,17 @@ module.exports = { | |||
| }); | |||
| } | |||
|
|
|||
| if (funcObject.vpcEgress) { | |||
There was a problem hiding this comment.
Why are we applying this only if vpcEgress is set directly on function? What is the purpose of provider level setting then? It will never be effective as it will be always overriden by value set on function or not applied at all
There was a problem hiding this comment.
Hey! sorry, I'm not following you here. What do you mean that the value will be overwritten?
I have the following configuration (https://i.imgur.com/7kENrcp.png) in my serverless.yml file. I think vpcEgress is configured at the function level.
I deployed using this fork and got the following result:
https://i.imgur.com/tb748wY.png
I wait your answer, thank you!
There was a problem hiding this comment.
What I mean here is that while we support vpcEgress setting on provider level, it will never take effect because the logic is applied only when the setting is applied directly on function. I would assume that if it's only set on provider level, then it should be applied to all functions in service, correct?
In the provided example you set the value on function level and that case is covered well, but if you'd like to apply it across your service and set it on provider.vpcEgress, then it just does not work at all.
There was a problem hiding this comment.
Hey! i think i got it, i just applied the change to make it through the provider level.
do you think it's ok?
thanks!
There was a problem hiding this comment.
Hello @fcr1193 👋 Thanks for update, unfortunatelly I don't think it's working at the moment, the tests are failing as I believe now function-level setting is fully ignored. Please see my comments and let me know what do you think
| @@ -57,6 +59,15 @@ module.exports = { | |||
| }); | |||
| } | |||
|
|
|||
| if (vpcEgress) { | |||
There was a problem hiding this comment.
It looks like now it ignores the setting on function level, is that the case or am I missing something here? It should override provider-level setting with function-specifc setting
| @@ -767,4 +767,129 @@ describe('CompileFunctions', () => { | |||
| }); | |||
| }); | |||
| }); | |||
|
|
|||
There was a problem hiding this comment.
It would be great to add test case that covers support for provider-level setting as well as how provider-level setting is overriden by more specific function-level configuration
|
Hey @pgrzesik how are you? I just added some updates, let me know what you think! thanks! |
package/lib/compileFunctions.js
Outdated
| @@ -20,11 +20,14 @@ module.exports = { | |||
| this.serverless.service.getAllFunctions().forEach((functionName) => { | |||
| const funcObject = this.serverless.service.getFunction(functionName); | |||
|
|
|||
| funcObject.vpcEgress = funcObject.vpcEgress || this.serverless.service.provider.vpcEgress; | |||
There was a problem hiding this comment.
Let's assign it to a new variable instead of mutating the original funcObject
| }); | ||
| }); | ||
|
|
||
| it('should replace vpc egress private at provider level for a vpc egress all defined at function level', () => { |
There was a problem hiding this comment.
I think we need just a single test for provider being overrided by function level setting. These further tests are not really needed so let's remove them
|
Hey @pgrzesik, how are you? thanks for your review, I just updated this! |
pgrzesik
changed the title
provider.vpcEgress configuration
Hey! I made some updates from this PR #250 (thanks @arei)
I would appreciate if you can give me feedback @pgrzesik
Thank you!