Fix lambda logical version id for layers in same stack #8066

pwithams · 2020-08-09T05:55:46Z

Closes: #8053

Problem

Lambda version ids were created only using the function properties. If a layer is referenced in the same stack, the property value (i.e. layer: [{'Ref': 'ExampleLambdaLayer'}]) will only change if the layer name is changed. This means that code changes in a layer will not trigger a new lambda version to be created.

Proposed fix

In addition to the function properties, the following have been added to the version id hash:

the contents of each referenced layer zip file
the properties of each referenced layer, except S3Key as it timestamped and therefore changes on every deploy

Note

I tried to avoid both bluebird and Lodash where possible but did use them in a few cases, especially where they were already being used. Unfortunately I am not a JS developer so any suggestions for where those can be removed or improvements in general are more than welcome.

Currently the lambda version logical id is created by detecting any changes in either the code or the lambda config. However, if the lambda uses a layer that has updated then there will be no change to the config as it just contains a reference to the resource name. To fix this, the layer details are extracted which will include the timestamped S3 layer zip name and therefore generate a new digest if the layer zip file name changes.

…ities

codecov-commenter · 2020-08-09T05:57:49Z

Codecov Report

Merging #8066 into master will increase coverage by 0.08%.
The diff coverage is 98.50%.

@@            Coverage Diff             @@
##           master    #8066      +/-   ##
==========================================
+ Coverage   88.02%   88.11%   +0.08%     
==========================================
  Files         248      248              
  Lines        9312     9338      +26     
==========================================
+ Hits         8197     8228      +31     
+ Misses       1115     1110       -5

Impacted Files	Coverage Δ
lib/plugins/aws/package/compile/functions/index.js	`97.29% <98.33%> (+0.26%)`	⬆️
lib/plugins/aws/package/compile/layers/index.js	`95.60% <100.00%> (ø)`
lib/plugins/aws/provider/awsProvider.js	`93.18% <100.00%> (+0.07%)`	⬆️
lib/plugins/package/lib/packageService.js	`90.16% <0.00%> (+4.09%)`	⬆️

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 73399cd...7c3ac73. Read the comment docs.

pwithams · 2020-08-09T06:13:51Z

The windows tests are failing, so I will look into that soon. My guess it that maybe the generated digests are different on windows, although there was a similar existing test 'should version only function that is flagged to be versioned'.

Either way, refactoring the tests to just compare the version id values rather than explicit matches is probably preferable so I will change that.

…it checks

pwithams · 2020-08-11T03:28:33Z

I have refactored the tests and added the windows compatibility fix. Let me know if there's anything else I should change.

medikoo

@pwithams great thanks for that! It's a very valuable improvement, please see my remarks

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

medikoo · 2020-08-17T10:34:50Z

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

+            // Include function and layer configuration in version id hash (without the Code part)
+            const functionProperties = _.omit(_.get(functionResource, 'Properties', {}), 'Code');
+            const layerProperties = this.extractLayerPropertiesFromFunction(
+              functionProperties,
+              cfTemplate
+            );

-          const fileDigest = fileHash.read();
-          const versionDigest = versionHash.read();
+            const mappedLayerFunctionProperties = layerProperties.map(layer => {
+              const clonedLayer = _.cloneDeep(layer);
+              clonedLayer.properties.Properties.Content.S3Key = '';
+              return clonedLayer.properties;
+            });


Let's reuse already resolved layerProperties (nice if named layerConfigurations) in previous block.

Also if I see correctly this logic simply removes S3Key properties, that can be done simpler way via:

for (const layerProperties of layerConfigurations) delete layerProperties.S3Key;

Let's reuse already resolved layerProperties (nice if named layerConfigurations) in previous block.

Doesn't seem to be addressed

If I understand correctly I think this should be covered by 14de011#diff-771a8504e96d8db95f70f2cca6e3a906R475

It doesn't seem. Note that layerConfigurations are still resolved twice in code as proposed:

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

Lines 452 to 455 in bccd773

const layerConfigurations = extractLayerConfigurationsFromFunction(

functionResource.Properties,

cfTemplate

);

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

Lines 476 to 479 in bccd773

const layerConfigurations = extractLayerConfigurationsFromFunction(

functionResource.Properties,

cfTemplate

);

Thanks, it is now moved out of the block and used twice.

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

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

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

The functions compile logic was refactored. The tests were updated to use the new runServerless utility. Additionally, old tests that used explicit hash references were updated to use comparisons instead as the compile logic now generates slightly different hashes.

pwithams · 2020-08-22T07:14:44Z

Thanks for the feedback! I think I've managed to address most of them but in doing so I ended up changing quite a bit, especially with the tests, so let me know if there is anything in these new changes I can improve.

medikoo

@pwithams great thanks for update! Still I think one of the most important suggestions I proposed was missed.

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

medikoo · 2020-08-25T13:21:38Z

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

-            } else {
-              versionHash.write(value != null ? String(value) : '');
+
+            return BbPromise.map(layerZipFilePaths, zipFileName => {


Let's not use Bluebird` functions which do not have native counterparts.

We should use BbPromise.all instead

This has been updated now, but I realized that both approaches in their current state will run asynchronously on the same hash object. I'm not sure if there could be a race condition or something if hash.write(chunk) calls on the same object collide.

A potential fix would be to rewrite addFileContentsToHash to return a new string or hash object representing the file. Once all promises finish, the array of strings/hashes could then be combined and added to the versionId hash. What do you think?

This has been updated now, but I realized that both approaches in their current state will run asynchronously on the same hash object.

That's true, it won't be deterministic then. We need to run it sequentially, I believe you can achieve it as:

return layerZipFilePaths.reduce( (previousHashCalculation, zipFileName) => previousHashCalculation.then(() => addFileContentsToHashes(zipFileName, [versionHash]), BbPromise.resolve() )

Note that also layerZipFilePaths order needs to also be deterministic

Thanks, this should be done now.

medikoo · 2020-08-25T13:22:46Z

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

+            // Include function and layer configuration in version id hash (without the Code part)
+            const functionProperties = _.omit(_.get(functionResource, 'Properties', {}), 'Code');
+            const layerProperties = this.extractLayerPropertiesFromFunction(
+              functionProperties,
+              cfTemplate
+            );

-          const fileDigest = fileHash.read();
-          const versionDigest = versionHash.read();
+            const mappedLayerFunctionProperties = layerProperties.map(layer => {
+              const clonedLayer = _.cloneDeep(layer);
+              clonedLayer.properties.Properties.Content.S3Key = '';
+              return clonedLayer.properties;
+            });


Let's reuse already resolved layerProperties (nice if named layerConfigurations) in previous block.

Doesn't seem to be addressed

medikoo

@pwithams great thanks for update. I believe we're definitely closer. See my remarks and let me know what you think

medikoo · 2020-08-25T13:25:22Z

tests/fixtures/functionLayersSourceChange/serverless.yml

@@ -0,0 +1,16 @@
+service: service


It'll be nice to introduce just one fixture

I wasn't sure exactly how to do this with one fixture. The test is to check that if only a line of the layer code is changed then a deploy will create a new lambda version that points to the new layer created. Everything else (serverless.yml, directory/file names etc.) all stays the same.

One way I considered was extending the fixture to point to a second layer directory that was slightly different. However, in that case the layer properties would be different and so it wouldn't recreate the exact scenario being tested for, i.e. the serverless.yml would be different.

I looked to see if there was a way I could update the code in the fixture's layer at runtime but that doesn't seem right. Any ideas?

So if I understand correctly, in test, we should modify same layer file and proceed with second deployment. It's important to not change the filename of a layer as that wouldn't properly reflect what we want to test.

If my understanding is correct, then maybe we could create two different layer versions (e.g. testLayer1, and testLayer2) directly in a fixture, and before first deployment copy testLayer1 into testLayer and before second deployment remove testLayer and copy testLayer2 into testLayer)

Cleanup of that can be done through fixtures.cleanup({ extraPaths: ['testLayer'])

Thanks, this should be done now.

medikoo

@pwithams that looks great! We're very close. New tests look very good. Please see my comments, it's last final remarks

medikoo · 2020-09-16T10:51:54Z

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

+  if (!functionProperties.Layers) return layerConfigurations;
+  functionProperties.Layers.forEach(potentialLocalLayerObject => {
+    if (potentialLocalLayerObject.Ref) {
+      const configuration = _.cloneDeep(cfTemplate.Resources[potentialLocalLayerObject.Ref]);


Why do we need to clone it?

Main reason was I assumed this line https://github.com/pwithams/serverless/blob/c8be293c97dd8550f2a5d02ee8d61e6032da3335/lib/plugins/aws/package/compile/functions/index.js#L513 would delete part of the already created cloudformation template if the reference was returned.

Also, in general it seemed like cfTemplate is such an important object that if part of it is being returned then it should be a copy so that anything using the return value doesn't accidentally alter it.

However, if the delete doesn't actually affect anything (I think removing the clone only fails one of my tests?) then it can be taken out, especially if lodash usage is being minimized.

cfTemplate is such an important object that if part of it is being returned then it should be a copy so that anything using the return value doesn't accidentally alter it.

In fact, if we're taking it as reference, then indeed we should never attempt to modify it. However implying a practice that whenever we take it, we copy it (just in case), could be noisy and difficult to follow, it's definitely not natural in JS.
It's more natural to purely stay away from modifying such reference objects.

So I'd say, let's not clone and let's ensure we do not modify it in further processing

That makes sense. I think it still has to be modified later in some way as S3Key can't be part of the hash otherwise it will change every time. I've now moved the clone out of the function and applied it to the returned result instead - this solves the configuration._serverlessLayerName naming issue below too.

Let me know if you have any other ideas how to handle keeping S3Key out of the hash without a clone.

medikoo · 2020-09-16T10:52:33Z

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

+      const configuration = _.cloneDeep(cfTemplate.Resources[potentialLocalLayerObject.Ref]);
+      layerConfigurations.push({
+        serverlessLayerName:
+          cfTemplate.Resources[potentialLocalLayerObject.Ref]._serverlessLayerName,


It could be configuration._serverlessLayerName

I think the reason I didn't use that is because cloneDeep only clones enumerable attributes. Not sure if there's another cleaner way to do it.

medikoo · 2020-09-16T10:56:39Z

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

          expect(
            awsCompileFunctions.serverless.service.provider.compiledCloudFormationTemplate.Outputs
-          ).to.deep.equal(expectedOutputs);
+          ).to.not.equal(firstOutputs);


I think we need to use deep.equal

I think the idea here was that the previous tests were relying on direct digest comparisons. So to support the new hash changes and make the tests avoid using hardcoded digests, more general comparisons were used.

Before, the test ran compileFunctions and checked that it gave a specific hardcoded output. Then it changed the input, updated the hardcoded expected output, and then checked that those new values matched.

Now the test just runs the first case, saves the first output, makes configuration changes, and then tests that the output to the second does not equal the first. To your point though I think it should probably be using to.not.deep.equal actually as Outputs is an object.

Does that make sense?

To your point though I think it should probably be using to.not.deep.equal actually as Outputs is an object.

Yes I actually meant to replace equal with deep.equal (so in result it's to.not.deep.equal). Sorry that was probably not clear enough

medikoo · 2020-09-16T10:57:47Z

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

-            .FuncLambdaVersionUF9qY6un3qUVhv62q1QPfZm4qaYxyEJXKjFTLIZ5Ig.Properties.Description
-        ).to.equal('Lambda function description');
+        )) {
+          if (key.includes('FuncLambdaVersion')) {


Slightly more precise would be startsWith

medikoo · 2020-09-16T11:01:35Z

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

+        awsRequestStubMap: {
+          CloudFormation: {
+            describeStacks: { Stacks: [{ Outputs: [{ OutputKey: 'test' }] }] },
+          },
+        },


It'll be nice to seclude this mock to var, and reuse it in each runServerless call

medikoo · 2020-09-16T11:08:29Z

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

+      await fse.rename(originalLayer, backupLayer);
+      await fse.rename(sourceChangeLayer, originalLayer);


To test it properly I believe we should change the layer path in a config (so point a file at different location with a same content).

Here we're replacing file with file of very same content, which technically brings no change in environement. which seems equal to not do any renames.

Yes re-reading this I think it is redundant. Changing the path in the config would also cause a version id change as the config is part of the hash. I think what I was trying to do here is test that if nothing is changed in the layer config (including source) then the version id stays the same.

This is kind of already covered in the should ignore changing character of S3Key paths when generating layer version id, but I've kept the test and simplified it to be should create same version id when layer source and config are unchanged and just ran runServerless twice. The benefit of keeping it would be to make clear cases where the S3Key test failed due to a more general and S3Key-unrelated issue.

medikoo · 2020-09-16T11:10:11Z

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

+      await fse.rename(originalLayer, sourceChangeLayer);
+      await fse.rename(backupLayer, originalLayer);


It would be nice to construct this with before and after, as otherwise if tests fail, the changed state will be leftover, possibly affecting results of other tests

Wasn't too sure exactly how to do this as the before and after need to be run specifically for this test only. I ended up using a nested describe with beforeEach and afterEach.

Yes, in that case it'll require nesting extra describe block (and imo it's nothing wrong with that)

medikoo · 2020-09-16T11:11:51Z

lib/plugins/aws/provider/awsProvider.js

+            '.serverless',
+            this.provider.naming.getLayerArtifactName(layerName)
+          );
+    return layerArtifactName;


I believe we can return resolved value directly (there seems no value in creating layerArtifactName variable)

medikoo · 2020-09-16T11:13:05Z

test/fixtures/functionLayers/serverless.yml

+  foo:
+    handler: index.handler
+    layers:
+      - { Ref: TestLayerLambdaLayer }
+  bar:
+    handler: index.handler
+  fooBar:
+    handler: index.handler
+    reservedConcurrency: 1


It'll be nice to give meaningful names (referencing what we test through them) to those lambdas, as we did with below ones

medikoo

@pwithams great thanks for update. It looks very good now!

I've just spotted one error, and also proposed few small improvements to use native interfaces on which we can rely on with v2 (as we've released it in a meantime)

medikoo · 2020-09-22T07:06:50Z

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

+    if (potentialLocalLayerObject.Ref) {
+      const configuration = cfTemplate.Resources[potentialLocalLayerObject.Ref];
+      layerConfigurations.push({
+        serverlessLayerName: configuration._serverlessLayerName,


I think we should name property name. Firstly in this context serverlessLayer prefix is not justified, and when processing it, in one place we assume it's name (

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

Line 419 in 76bf822

for (const { name, properties: layerProperties } of layerConfigurations) {

) which now resolves to undefined

Thanks, good catch. I think it must have been name at one point, so I've changed it back. I've added the other improvements and merged in this change from master (1fceb89) so let me know me if anything looks wrong with that.

medikoo · 2020-09-22T07:10:59Z

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

+            return layerArtifactPaths
+              .sort()
+              .reduce(
+                (previousHashCalculation, artifactName) =>
+                  previousHashCalculation.then(() =>
+                    addFileContentsToHashes(artifactName, [versionHash])
+                  ),
+                BbPromise.resolve()
+              );


Since we're on v2 we can safely rely on async/await and native promises, so now we can construct it way more simply as:

for (const layerArtifactPath of layerArtifactPaths) { await addFileContentsToHashes(layerArtifactPath, [versionHash]) }

Can we refactor that one, to form as proposed in above comment?

I had refactored but I'd forgotten to remove the old logic... it should be fixed now.

medikoo · 2020-09-22T07:11:48Z

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

+            const sortedLayerConfigurations = {};
+            const byKey = ([key1], [key2]) => key1.localeCompare(key2);
+            for (const { name, properties: layerProperties } of layerConfigurations) {
+              sortedLayerConfigurations[name] = _.fromPairs(_.entries(layerProperties).sort(byKey));


Let's use Object.entries instead of _.entries (as in a meantime we've dropped support for Node.js v8)

medikoo · 2020-09-22T07:11:55Z

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


-          const versionResource = this.cfLambdaVersionTemplate();
+            const sortedFunctionProperties = _.fromPairs(_.entries(functionProperties).sort(byKey));


medikoo · 2020-09-22T07:12:42Z

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

@@ -623,4 +646,38 @@ class AwsCompileFunctions {
  }
 }

+function addFileContentsToHashes(filePath, hashes) {
+  return new BbPromise((cb, reject) => {


Let's use native Promise, and for consistency let's use resolve name instead cb

medikoo

@pwithams looks great! One last, thing, can we have that reduce construct refactored to simple for.. of await ? See my comment, in previous review, I've proposed on how it can look.

Thank you!

medikoo · 2020-09-23T11:23:48Z

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

+            return layerArtifactPaths
+              .sort()
+              .reduce(
+                (previousHashCalculation, artifactName) =>
+                  previousHashCalculation.then(() =>
+                    addFileContentsToHashes(artifactName, [versionHash])
+                  ),
+                BbPromise.resolve()
+              );


Can we refactor that one, to form as proposed in above comment?

medikoo

Thank you @pwithams ! We finally have it, and it seems very solid

pwithams · 2020-09-25T00:45:04Z

Thank you @pwithams ! We finally have it, and it seems very solid

🎉 Thanks for the help and patience @medikoo!

pwithams added 8 commits August 5, 2020 17:41

Enable change by removing empty array

34b0877

Add version id change based on layer zip content and non-S3Key proper…

91e3077

…ities

Fix version id creation logic to pass existing tests

f927470

Add tests for new lambda version layer logic

f93b1aa

Merge branch 'master' into fix-logical-version-id-for-layers

b4ee79a

Remove some lodash uses

3a01dc9

Add test to check for reservedConcurrency version id change

b51b81e

pwithams added 3 commits August 9, 2020 23:00

Refactor first test to use version id comparisions rather than explic…

6d412bc

…it checks

Update tests to use comparisons rather than explicit checks

b5cb580

Use path.join to create test file paths

3080dd3

medikoo suggested changes Aug 17, 2020

View reviewed changes

medikoo added bug cat/lambda labels Aug 17, 2020

medikoo assigned pwithams Aug 17, 2020

pwithams added 4 commits August 21, 2020 23:01

Add test to check for version update on key change

001f440

Merge branch 'master' into fix-logical-version-id-for-layers

8678062

Fix windows tests

619050c

medikoo reviewed Aug 24, 2020

View reviewed changes

lib/plugins/aws/package/compile/functions/index.js Show resolved Hide resolved

Use correct layer reference syntax

781aa98

medikoo reviewed Aug 25, 2020

View reviewed changes

medikoo suggested changes Aug 25, 2020

View reviewed changes

pwithams added 3 commits August 25, 2020 19:49

Improve artifact path detection logic

a6ccbac

Change promise map to all and update reservedConcurrency test

3ad5651

Clean up function fixture use in reservedConcurrency test

bccd773

pwithams added 7 commits September 13, 2020 11:39

Simplfy naming of hidden serverless layer name object

d7704ec

Merge branch 'master' into fix-logical-version-id-for-layers

b1571c3

Move layer artifact name resolution into separate method

9ee00b4

Update tests to use async/await and new fixture engine

38c867e

Merge branch 'master' into fix-logical-version-id-for-layers

ff09d04

Use fs-extra for async/await functionality

6e0be19

Add test case for function with arn layer reference

193c516

pwithams requested a review from medikoo September 16, 2020 02:50

medikoo suggested changes Sep 16, 2020

View reviewed changes

pwithams added 5 commits September 18, 2020 18:10

Merge branch 'master' into fix-logical-version-id-for-layers

7ad4818

Improve naming and refactor tests

9e08cf5

Fix equality check used in refactored existing test

c8be293

Move object clone logic out of function

eee300e

Merge branch 'master' into fix-logical-version-id-for-layers

76bf822

pwithams requested a review from medikoo September 22, 2020 02:14

medikoo suggested changes Sep 22, 2020

View reviewed changes

pwithams added 2 commits September 22, 2020 17:55

Merge master and fix conflicts

a9baf17

Fix naming issue and increase native interface usage

6967213

pwithams requested a review from medikoo September 23, 2020 00:18

medikoo reviewed Sep 23, 2020

View reviewed changes

pwithams added 2 commits September 23, 2020 17:06

Removed refactored logic

b3cea34

Merge branch 'master' into fix-logical-version-id-for-layers

7c3ac73

pwithams requested a review from medikoo September 23, 2020 23:17

medikoo approved these changes Sep 24, 2020

View reviewed changes

medikoo merged commit e43c889 into serverless:master Sep 24, 2020

medikoo mentioned this pull request Oct 13, 2020

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

Closed

Enase mentioned this pull request Oct 13, 2020

"A version for this Lambda function exists ( 8 ). Modify the function to create a new version.." even when functions have changed #8392

Closed

pgrzesik mentioned this pull request Aug 9, 2021

Issue in adding provisionedConcurrency or functionVersion set to true, Could not find reference to layer created in Resource section #9797

Closed

medikoo mentioned this pull request Oct 28, 2021

feat: Allow legacy option for lambdaHashingVersion property #10173

Merged

	const layerConfigurations = extractLayerConfigurationsFromFunction(
	functionResource.Properties,
	cfTemplate
	);

		await fse.rename(originalLayer, backupLayer);
		await fse.rename(sourceChangeLayer, originalLayer);

		await fse.rename(originalLayer, sourceChangeLayer);
		await fse.rename(backupLayer, originalLayer);


		const versionResource = this.cfLambdaVersionTemplate();
		const sortedFunctionProperties = _.fromPairs(_.entries(functionProperties).sort(byKey));

Fix lambda logical version id for layers in same stack #8066

Fix lambda logical version id for layers in same stack #8066

Conversation

pwithams commented Aug 9, 2020 • edited by medikoo

Problem

Proposed fix

Note

codecov-commenter commented Aug 9, 2020 • edited

Codecov Report

pwithams commented Aug 9, 2020

pwithams commented Aug 11, 2020

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

pwithams commented Aug 22, 2020

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

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

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

pwithams Sep 19, 2020 • edited

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwithams Sep 23, 2020 • edited

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

pwithams commented Sep 25, 2020

pwithams commented Aug 9, 2020 •

edited by medikoo

codecov-commenter commented Aug 9, 2020 •

edited

pwithams Sep 19, 2020 •

edited

pwithams Sep 23, 2020 •

edited