Retain console properties on function deploy

[Danwakeem]

Description

Fixed an issue when deploying a single function that has been instrumented by serverless console.

Should retain environment variables and layers externally added by serverless console 👍

[codecov]

Codecov Report

Patch coverage: 95.91% and project coverage change: +0.66 🎉

Comparison is base (9c7e5b4) 85.97% compared to head (e88a482) 86.64%.

❗ Current head e88a482 differs from pull request most recent head a1e711a. Consider uploading reports for the commit a1e711a to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #11849      +/-   ##
==========================================
+ Coverage   85.97%   86.64%   +0.66%     
==========================================
  Files         316      316              
  Lines       13377    13401      +24     
==========================================
+ Hits        11501    11611     +110     
+ Misses       1876     1790      -86     

Impacted Files	Coverage Δ
lib/classes/utils.js	95.74% <ø> (ø)
lib/classes/yaml-parser.js	100.00% <ø> (ø)
lib/plugins/aws/deploy-function.js	96.53% <94.11%> (+0.31%)	⬆️
lib/cli/interactive-setup/console-dev-mode-feed.js	87.59% <100.00%> (+69.85%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[medikoo]

Thanks @Danwakeem for taking care of that.

It'll be great to also have that covered with unit tests (setup with runServerless) that test scenario where layers are configured remotely and (1) not configured explicitly in service config (2) configured explicitly in service config

[Danwakeem]

@medikoo I noticed that the updateFunctionConfiguration configuration was stubbed in the unit tests. Would you mind if I extracted out the update layers and environment variable logic and unit test those functions individually?

I feels like there is a lot going on in the updateFunctionConfiguration and it might be worth breaking out a few of these steps into their own functions so they are easily testable. What do you think?

[medikoo]

@Danwakeem ideally this should be handled as other tests that are configured with runServerless.

What you need to here is believe introduce some extra representations of lambda configurations to be returned by Lambda.getFunction. This should be pretty easy I think. It's a question of reconfiguring this:

serverless/test/unit/lib/plugins/aws/deploy-function.test.js

Lines 326 to 332 in 77c91f2

	getFunction: {
	Configuration: {
	LastModified: '2020-05-20T15:34:16.494+0000',
	State: 'Active',
	LastUpdateStatus: 'Successful',
	},
	},

Into dynamic resolution as follows:

getFunction: (params) {
  if (params.FunctionName === 'console') {
    // return configuration with layers
  } else {
    return {
          LastModified: '2020-05-20T15:34:16.494+0000',
          State: 'Active',
          LastUpdateStatus: 'Successful',
        };
  }
}

And then in your test cases when you want to test against functions with layers you just pick name which will return you configurations with layers

[Danwakeem]

@medikoo But isn't the function this code is written in stubbed in those same tests? So runServerless wouldn't run the logic in the updateFunctionCode? 🤔 👇

serverless/test/unit/lib/plugins/aws/deploy-function.test.js

Line 334 in 77c91f2

updateFunctionConfiguration: updateFunctionConfigurationStub,

If this is the case, it might make sense to just extract the two logic blocks where we merge layers and environment variables into small functions so we can test those bits of logic individually. I was thinking this could make this block of code a bit more readable and easier to test.

[medikoo]

updateFunctionConfigurationStub is a stub for AWS SDK Function.updateFunctionConfiguration request, so no real request is issued but you can track with what arguments it was called. We do not provide any internal logic for this stub, it's a pure sinon stub.

In test I believe you need to confirm that this stub was triggered with expected layers set and environment variables

[Danwakeem]

Thanks @medikoo. After poking around with the tests for a bit I see how these fit together. I ended up adding the tests you had suggested and some more fine grain tests on merging environment and layer properties.

Let me know what you think about the tests I added in the most recent commit 🙏

[medikoo]

Thank you @Danwakeem. It looks very promising. We're getting close 👍

[Danwakeem]

@medikoo Thanks for pointing out that I could convert the getFunction property to a function. It would have taken me a while to figure that out 😅

[medikoo]

Thank you @Danwakeem ! I have just a few final suggestions

[medikoo]

Thanks @Danwakeem, we're close!

[medikoo]

Looks great 👍 🎉

[Danwakeem]

Thanks again @medikoo 🎸 lol I think with all the help you have offered me I owe you a beer or something 🍻