exclude tags and reservedConcurrency from version hash #8212

thewizarodofoz · 2020-09-10T08:16:06Z

Closes: #7822

codecov-commenter · 2020-09-10T08:18:41Z

Codecov Report

Merging #8212 into master will decrease coverage by 0.12%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #8212      +/-   ##
==========================================
- Coverage   88.16%   88.03%   -0.13%     
==========================================
  Files         248      248              
  Lines        9430     9320     -110     
==========================================
- Hits         8314     8205     -109     
+ Misses       1116     1115       -1

Impacted Files	Coverage Δ
lib/plugins/aws/package/compile/functions/index.js	`97.03% <100.00%> (-0.22%)`	⬇️
...plugins/aws/package/compile/events/stream/index.js	`98.00% <0.00%> (-0.60%)`	⬇️
.../package/compile/events/websockets/lib/validate.js	`97.22% <0.00%> (-0.51%)`	⬇️
lib/configSchema.js	`100.00% <0.00%> (ø)`
lib/utils/getCommandSuggestion.js	`100.00% <0.00%> (ø)`
lib/plugins/aws/invokeLocal/index.js	`69.66% <0.00%> (ø)`
lib/plugins/aws/provider/awsProvider.js	`93.11% <0.00%> (ø)`
lib/plugins/interactiveCli/initializeService.js	`100.00% <0.00%> (ø)`
...ib/plugins/aws/package/compile/events/sns/index.js	`100.00% <0.00%> (ø)`
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3177e40...e29da47. Read the comment docs.

medikoo

Thank you @thewizarodofoz ! That looks very promising. Please see my remarks

medikoo · 2020-09-10T08:36:07Z

lib/plugins/aws/package/compile/functions/index.js

+          const functionWideProperties = ['ReservedConcurrentExecutions', 'Tags'];
+          const properties = _.omit(
+            _.get(functionResource, 'Properties', {}),
+            'Code',
+            ...functionWideProperties
+          );


What's the added value of creating functionWideProperties variable.

Isn't it simpler to just do _.omit(..., ['Code', 'ReservedConcurrentExecutions', 'Tags'] ?

I thought it would make it more clear as to why these fields are omitted

@thewizarodofoz that's a good point , but it'll probably be fine to solve it with simple inline code comment:

_.omit(..., [ 'Code', // Properties applied to function globally (not specific to version or alias) 'ReservedConcurrentExecutions', 'Tags' ])

medikoo · 2020-09-10T08:46:57Z

lib/plugins/aws/package/compile/functions/index.test.js

@@ -2240,6 +2240,55 @@ describe('AwsCompileFunctions', () => {
        });
    });

+    it('should not create a new version object if only function-wide configuration changed', () => {


All new tests should be written with help of runServerless util (see https://github.com/serverless/serverless/blob/master/test/README.md)

In this case I would rely on basic function fixture as follows

Setup a fixture with fixture.setup, as e.g. it's done here:

serverless/lib/utils/analytics/generatePayload.test.js

Line 18 in d9b91e9

fixtures.setup('httpApi').then(({ servicePath }) =>

Issue first runServerless and read all generated lambda version ids

Modify reservedConcurrency and tags with help of updateConfig (returned by fixture.setup()) as it's done here:

serverless/test/integration/apiGateway.test.js

Lines 191 to 204 in d9b91e9

await updateConfig({

provider: {

tags: {

foo: 'bar',

baz: 'qux',

},

tracing: {

apiGateway: true,

},

logs: {

restApi: true,

},

},

});

Re-run runServerless, and confirm that version id didn't change

Thanks @medikoo I refactored the test, but it seems to me the second call to runServerless isn't reading the updated config. I debugged the test and I did see updateConfig updating the serverless.yml in the temp dir, but as you can see in this assertion (which fails), runServerless isn't picking up those changes.

medikoo

runServerless isn't picking up those changes.

Thanks for pointing. Indeed there was an issue in memoization setup, due to which, in same process second serverless instance of same service was relying on previously resolved config. I've fixed that in #8231

Update with master and issue should be gone

medikoo · 2020-09-14T10:41:01Z

lib/plugins/aws/package/compile/functions/index.js

+            _.get(functionResource, 'Properties', {}),
+            'Code',
+            // Properties applied to function globally (not specific to version or alias)
+            // https://github.com/serverless/serverless/issues/7822


I think we can remove the link to github issue.

If needed, it's easy to map given code lines to source PR and issue, by GitHub blame feature

…ide-properties-in-version-hash

medikoo

@thewizarodofoz that looks great! Thank you

I have just one final remark. Last week we've released v2 and dropped support for Node.js v6, which means we can freely now use async/await syntax, and ideally (for better maintenance) if all new code is introduced in that style.

Can you update the fixture so it's configured with async function and await keywords?

Sorry for late notice

medikoo · 2020-09-15T08:30:22Z

lib/plugins/aws/package/compile/functions/index.test.js

+    it('should not create a new version object if only function-wide configuration changed', () => {
+      return fixtures.setup('function').then(({ servicePath, updateConfig }) =>


Can we upgrade this to async/await syntax?

I was really hoping to do that! Yay!

medikoo · 2020-09-21T16:06:29Z

@thewizarodofoz is this ready for re-review? If so, please re-request review in Reviewers box

medikoo

Thank you @thewizarodofoz, It's all great, aside of that we should not commit describe.only.

Can you update it?

medikoo · 2020-09-22T06:39:32Z

lib/plugins/aws/package/compile/functions/index.test.js

@@ -2831,4 +2832,38 @@ describe('AwsCompileFunctions #2', () => {
      });
    });
  });
+
+  describe.only('function config', () => {


.only I believe was committed in by accident

medikoo

Thank you @thewizarodofoz !

azizur · 2020-09-27T17:34:35Z

This PR has not resolved the issue. I am still getting A version for this Lambda function exists ( XXX ). Modify the function to create a new version..

medikoo · 2020-09-28T11:10:48Z

@azizur see: #7822 (comment)

ozwi added 2 commits September 10, 2020 10:50

exclude tags and reservedConcurrency from version hash

ecc172c

push prettify changes

c1adb6a

medikoo suggested changes Sep 10, 2020

View reviewed changes

medikoo added bug cat/lambda cat/packaging labels Sep 10, 2020

medikoo assigned thewizarodofoz Sep 10, 2020

ozwi added 3 commits September 11, 2020 12:21

refactor test - use runServerless

8562f4f

replace variable with comment

19ab38c

better comment

6efc191

medikoo mentioned this pull request Sep 14, 2020

Fix config file resolution memoization #8231

Merged

medikoo reviewed Sep 14, 2020

View reviewed changes

ozwi added 4 commits September 14, 2020 15:50

Merge remote-tracking branch 'upstream/master' into ignore-function-w…

5f0b585

…ide-properties-in-version-hash

remove pr link

b424b3c

prettify

613e737

revert .only

16be040

thewizarodofoz requested a review from medikoo September 15, 2020 07:40

medikoo reviewed Sep 15, 2020

View reviewed changes

ozwi added 2 commits September 15, 2020 11:46

async/await!!!

b8dee0e

async/await!!! - for real

39e31eb

thewizarodofoz mentioned this pull request Sep 15, 2020

Lambda Version fails to publish when CloudFormation changes but Lambda config doesn't #8251

Closed

thewizarodofoz requested a review from medikoo September 21, 2020 18:00

medikoo suggested changes Sep 22, 2020

View reviewed changes

remove .only

e29da47

thewizarodofoz requested a review from medikoo September 22, 2020 07:20

medikoo approved these changes Sep 22, 2020

View reviewed changes

medikoo merged commit 1fceb89 into serverless:master Sep 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

exclude tags and reservedConcurrency from version hash #8212

exclude tags and reservedConcurrency from version hash #8212

thewizarodofoz commented Sep 10, 2020

codecov-commenter commented Sep 10, 2020 •

edited

medikoo left a comment

medikoo Sep 10, 2020

thewizarodofoz Sep 10, 2020

medikoo Sep 10, 2020

medikoo Sep 10, 2020

thewizarodofoz Sep 11, 2020

medikoo left a comment

medikoo Sep 14, 2020

medikoo left a comment

medikoo Sep 15, 2020

thewizarodofoz Sep 15, 2020

medikoo commented Sep 21, 2020

medikoo left a comment

medikoo Sep 22, 2020

medikoo left a comment

azizur commented Sep 27, 2020

medikoo commented Sep 28, 2020

	await updateConfig({
	provider: {
	tags: {
	foo: 'bar',
	baz: 'qux',
	},
	tracing: {
	apiGateway: true,
	},
	logs: {
	restApi: true,
	},
	},
	});

		it('should not create a new version object if only function-wide configuration changed', () => {
		return fixtures.setup('function').then(({ servicePath, updateConfig }) =>

exclude tags and reservedConcurrency from version hash #8212

exclude tags and reservedConcurrency from version hash #8212

Conversation

thewizarodofoz commented Sep 10, 2020

codecov-commenter commented Sep 10, 2020 • edited

Codecov Report

medikoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medikoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medikoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medikoo commented Sep 21, 2020

medikoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medikoo left a comment

Choose a reason for hiding this comment

azizur commented Sep 27, 2020

medikoo commented Sep 28, 2020

codecov-commenter commented Sep 10, 2020 •

edited