fix(Packaging): Consider absolute artifact paths (#8325) #8315

rib · 2020-10-01T01:23:10Z

Closes: #8325
Addresses #6752

Would also close softprops/serverless-rust#82

Although it looks like there may be more than one trigger for issue #6752, this change addresses one case seen as a result of the serverless-rust plugin setting artifacts with an absolute path which wasn't considered by lib/packageService.js.

An alternative fix might change serverless-rust to set a relative path, but this seems like a more general fix.

codecov-commenter · 2020-10-01T01:25:20Z

Codecov Report

Merging #8315 (0b1992e) into master (cf7b25e) will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #8315      +/-   ##
==========================================
+ Coverage   87.29%   87.42%   +0.13%     
==========================================
  Files         256      254       -2     
  Lines        9655     9615      -40     
==========================================
- Hits         8428     8406      -22     
+ Misses       1227     1209      -18

Impacted Files	Coverage Δ
lib/plugins/package/lib/packageService.js	`96.42% <100.00%> (+5.35%)`	⬆️
.../classes/ConfigSchemaHandler/normalizeAjvErrors.js	`95.72% <0.00%> (-0.08%)`	⬇️
lib/plugins/aws/lib/cloudformationSchema.js
...classes/ConfigSchemaHandler/resolveDataPathSize.js
lib/classes/ConfigSchemaHandler/index.js	`91.41% <0.00%> (+0.05%)`	⬆️
lib/classes/PluginManager.js	`97.29% <0.00%> (+0.67%)`	⬆️
lib/plugins/aws/invoke.js	`98.21% <0.00%> (+1.78%)`	⬆️
lib/plugins/aws/deployFunction.js	`95.00% <0.00%> (+5.83%)`	⬆️

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 cf7b25e...0b1992e. Read the comment docs.

rib · 2020-10-01T01:43:26Z

I spent a bunch of time trying to figure out how to run the prettier check but first it wasn't running after installing via npm as described at https://prettier.io/docs/en/install.html and then I hit various config file issues ¯_(ツ)_/¯ so gonna give up with that, sorry.

The change is trivial, and I'm not at all opinionated about it, so hopefully if it looks like a helpful fix someone will be able to handle whatever formatting tweaks are necessary, or rewrite, or find a different approach etc.

medikoo

@rib thanks for this PR, still this doesn't look related to #6752 (which looks as a result of issue in plugin which blindly assumes existence of function[].package)

While softprops/serverless-rust#82 seems also an error in plugin which for some reason assigns absolute and not relative path to package.artifact

Framework intentionally expects a relative path, as relying on absolute paths doesn't make service portable.

rib · 2020-10-01T14:40:50Z

:-( Please at least check the last comment I wrote for #6752 which explains in detail how I arrived at this change last night. Saying thanks "this doesn't look related to #6752" is kinda frustrating from the pov of someone that spent their evening debugging why I've not been able to deploy single functions for a while now (which is what that issue is about). The patch literally fixes the issue for me. It also feels like your bouncing back my own debugging work at me by telling me the cause of softprops/serverless-rust#82 which I posted yesterday and also suggested that it could be good for serverless-rust to set a relative path.

I marked this as Addresses #6752 not Closes because I see that there are clearly multiple things causing the same issue (I wouldn't have known that before spending the time to debug and investigate exactly what the cause of the bug was)

My initial impression was that it might be more robust to remove the assumption about relative paths considering that I didnt see any documentation that explicitly clarified that requirement and it seemed plausible that the exact same issue might just crop up again later if it was only addressed by changing the serverless-rust plugin.

Sorry if I sound grumpy - I think I was caught off guard by how dismissive the last comment was.

It sounds fine to me if serverless only wants to support relative artifact paths and change serverless-rust accordingly (service portability sounds like a good reason). Personally I think if you decide that then I think it might still be worth considering adding a check for absolute paths that at least shows a warning/error to avoid having this issue easily repeat itself in the future.

medikoo · 2020-10-01T15:38:41Z

:-( Please at least check the last comment I wrote for #6752 which explains in detail how I arrived at this change last night.

@rib I've read it, and what you've experienced does not look related to original #6752 report, and I've explained why I think so.

The patch literally fixes the issue for me.

I believe that issues should be fixed at the source, and not in whatever way that makes issue "gone" for given specific case.

Here seems the problem is in a plugin which without a valid reason produces an absolute path (note it may trigger some other issues which we're not aware of)

Personally I think if you decide that then I think it might still be worth considering adding a check for absolute paths that at least shows a warning/error to avoid having this issue easily repeat itself in the future.

That's a very good idea. We probably should add such validation inline, directly in logic, as my guess is that plugin sets this property dynamically at runtime, so validating that just by property schema will not work (?)

rib · 2020-10-01T17:05:03Z

I've filed #8325 to track the more specific issue I ended up hitting

medikoo

While it's hard to find a good reason for absolute paths, after all there's no harm in supporting them, and current behavior where invalid path is produced is clearly wrong.

I've proposed on how to solve it more cleanly

medikoo · 2020-10-02T09:52:40Z

lib/plugins/package/lib/packageService.js

  packageFunction(functionName) {
    const functionObject = this.serverless.service.getFunction(functionName);
    const funcPackageConfig = functionObject.package || {};

    // use the artifact in function config if provided
    if (funcPackageConfig.artifact) {
-      const filePath = path.join(this.serverless.config.servicePath, funcPackageConfig.artifact);
+      const filePath = this.absoluteArtifactPath(funcPackageConfig.artifact);


Let' simply just use path.resolve instead of path.join (that alone will ensure that absolute paths are respected)

ah, yep this looks good to me. I don't work with node.js often and was initially surprised that path.join didn't work that way by default - more like python's path.join which would have discarded the first absolute path automatically.

rib · 2020-10-02T17:01:52Z

Ah, so path.resolve does a bit more than just handle combinations of absolute paths, it will finally combine with the current working directory if the combination still isn't an absolute path and so using path.resolve sort of takes it to an extreme where artifact paths will now always be absolute paths. This requires some unit test updates too - maybe that's ok, or we could potentially go back to the previous version which was a bit more conservative and passed the current unit tests?

medikoo · 2020-10-05T13:07:53Z

@rib Tests fail cause of issue in tests, and not that we've introduced a bug. They implied a non absolute servicePath, which I believe, in real world is never the case.

So best way to follow would be to just update tests to mock servicePath with absolute path (we can simply prefix them with /).

As a side note, we configure all new tests with help of runServerless, util, to avoid side effects like that one.

medikoo · 2020-10-29T10:30:27Z

@rib what is the status of that? Do you think you can finish it? We'd love to have that in

pgrzesik · 2020-12-20T20:01:38Z

Hello @rib , thanks a lot for your contributions to this PR 🙇 We'd like to wrap it up in the coming days, so I hope you don't mind me taking over your work and applying the finishing changes. Please let me know if you'd still like to continue your work here. Once again, your help is much appreciated 🙇

rib · 2020-12-20T20:18:55Z

Yep, that's no problem @pgrzesik, sorry I haven't had time to follow up on this. thanks!

pgrzesik · 2020-12-30T12:01:37Z

lib/plugins/package/lib/packageService.js

        this.serverless.config.servicePath,
        this.serverless.service.package.artifact
      );
      funcPackageConfig.artifact = filePath;
+      functionObject.package = funcPackageConfig;


This change is a bit controversial as it's changed only because I needed a way to perform an assertion in tests for service-wide scenario - do you think there's another way of doing it without introducing extra production code?

Why exactly do we need it?

Technically in tests we should purely test the outcome, so either inspect generated artifact, or generated CF template

Would you suggest to assert on Properties.Code.S3Key on CF function resources instead here?

Would you suggest to assert on Properties.Code.S3Key on CF function resources instead here?

As I think more of it, that will also not work, as we just blindly assign there basename of artifact. So fact that full path is broken won't be exposed.

If I understand correctly we're fixing here issue that happens during deployment, where for upload to S3 we try to reach wrong path.

In light of that probably best test is one that tests against sls deploy and sls deploy function commands and confirms that we attempt to upload expected content to S3.

I think we can construct it by mocking AWS SDK through awsRequestStubMap option, and by setting lastLifecycleHookName to avoid full deployment process.
e.g. we have test that does something similar here:

serverless/test/unit/lib/plugins/aws/deploy/lib/checkForChanges.test.js

Line 1010 in ef53050

lastLifecycleHookName: 'aws:deploy:deploy:checkForChanges',

As hint you may inspect what exactly lifecycle events are triggered in which order by running deployment with debug branch (I've just ensured it's up to date with master). It logs all meta about that (we will put into core with regular debug logs after we have #1720 addressed)

For sls deploy function we're just passing artifact data to updateFunctionCode, so I've went this route for these tests (ensuring that AWS API is called with correct content). As for sls deploy, I believe it's not affected by this issue, but I'll try to come up with meaningful tests for that scenario as well

I've adjusted the approach a bit as confirming on the content was a bit cumbersome in case of readStream, so I've decided to confirm that the stream is created based on proper path to artifact

pgrzesik · 2021-01-05T09:38:53Z

@medikoo Would you be able to take a second look whenever you got a chance?

medikoo

@pgrzesik looks great! I have just few questions, please see my comments

medikoo · 2021-01-05T14:50:06Z

test/unit/lib/plugins/package/lib/packageService.test.js

            },
          },
        },
      });
    });
-    it('should support `package.artifact`', () => {
+
+    it.skip('should support `package.artifact`', () => {


Let's add TODO: prefix to message

medikoo · 2021-01-05T15:51:01Z

test/unit/lib/plugins/package/lib/packageService.test.js

+          });
+
+          // One with CF template, one with function artifact and one with provided function artifact
+          expect(s3UploadStub).to.be.calledThrice;


Do you think it's important to confirm on that (?) I guess there are some unrelated uploads involved there

I wanted to differentiate this way between the two cases (for service-wide scenario there's one upload less), but it's not essential I think

It looks as implementation detail, which may change without breaking functionality (e.g. in some other unrelated part we'll do S3 upload and it'll break tests here), so i wouldn't cover this.

makes sense, removed 👍

medikoo · 2021-01-05T15:52:46Z

test/unit/lib/plugins/package/lib/packageService.test.js

+        absoluteArtifactFilePath = path.join(process.cwd(), 'newArtifact.zip');
+        await fs.promises.writeFile(absoluteArtifactFilePath, zipContent);


Maybe we can reuse artifact directly from a fixture (we may eventually copy it to a new path, assuming that we rely on filename to confirm that things works)

medikoo

Looks great! 👍

medikoo reviewed Oct 1, 2020

View reviewed changes

medikoo added the needs feedback label Oct 1, 2020

medikoo assigned rib Oct 1, 2020

rib mentioned this pull request Oct 1, 2020

Artifact path in serverless-state.json is incorrect #5645

Open

This was referenced Oct 1, 2020

Cannot deploy selected functions softprops/serverless-rust#82

Open

No recognition of absolute artifact paths - e.g. breaks single function deployment #8325

Closed

medikoo suggested changes Oct 2, 2020

View reviewed changes

medikoo added bug cat/packaging and removed needs feedback labels Oct 2, 2020

rib force-pushed the wip/rib/single-func-deploy-6752 branch from ef758d1 to 904c2c1 Compare October 2, 2020 16:16

rib changed the title ~~fix(packaging): Consider absolute artifact paths (#6752)~~ fix(packaging): Consider absolute artifact paths (#8325) Oct 2, 2020

medikoo added the help wanted label Nov 30, 2020

medikoo assigned pgrzesik and unassigned rib Dec 16, 2020

pgrzesik force-pushed the wip/rib/single-func-deploy-6752 branch from 904c2c1 to 4e1d587 Compare December 22, 2020 16:41

pgrzesik force-pushed the wip/rib/single-func-deploy-6752 branch from 6ef14f4 to 212b412 Compare December 30, 2020 11:47

pgrzesik reviewed Dec 30, 2020

View reviewed changes

pgrzesik requested a review from medikoo December 30, 2020 12:01

pgrzesik force-pushed the wip/rib/single-func-deploy-6752 branch from 212b412 to 88df226 Compare December 30, 2020 16:27

pgrzesik force-pushed the wip/rib/single-func-deploy-6752 branch from 88df226 to e3bb77c Compare December 30, 2020 16:32

rib and others added 5 commits January 4, 2021 21:26

fix(packaging): Consider absolute artifact paths (serverless#8325)

40bf2a8

Add tests

645b2f1

ensure that proper content is uploaded

022f15c

add tests for service-wide deploy

5104516

reformat

943b75c

pgrzesik force-pushed the wip/rib/single-func-deploy-6752 branch from c35f779 to 943b75c Compare January 5, 2021 07:56

medikoo reviewed Jan 5, 2021

View reviewed changes

pgrzesik force-pushed the wip/rib/single-func-deploy-6752 branch from 2a3f6cf to eaf74cf Compare January 7, 2021 07:44

pgrzesik requested a review from medikoo January 7, 2021 07:52

use fixture with artifact

0b1992e

pgrzesik force-pushed the wip/rib/single-func-deploy-6752 branch from eaf74cf to 0b1992e Compare January 7, 2021 12:25

medikoo approved these changes Jan 7, 2021

View reviewed changes

pgrzesik changed the title ~~fix(packaging): Consider absolute artifact paths (#8325)~~ fix(Packaging): Consider absolute artifact paths (#8325) Jan 7, 2021

pgrzesik merged commit bcbbd47 into serverless:master Jan 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(Packaging): Consider absolute artifact paths (#8325) #8315

fix(Packaging): Consider absolute artifact paths (#8325) #8315

rib commented Oct 1, 2020 •

edited

Loading

codecov-commenter commented Oct 1, 2020 •

edited by codecov bot

Loading

rib commented Oct 1, 2020

medikoo left a comment

rib commented Oct 1, 2020

medikoo commented Oct 1, 2020

rib commented Oct 1, 2020

medikoo left a comment

medikoo Oct 2, 2020

rib Oct 2, 2020

rib commented Oct 2, 2020

medikoo commented Oct 5, 2020

medikoo commented Oct 29, 2020

pgrzesik commented Dec 20, 2020

rib commented Dec 20, 2020

pgrzesik Dec 30, 2020

medikoo Dec 30, 2020

pgrzesik Dec 30, 2020

medikoo Dec 30, 2020

pgrzesik Dec 30, 2020

pgrzesik Jan 4, 2021

pgrzesik commented Jan 5, 2021

medikoo left a comment

medikoo Jan 5, 2021

medikoo Jan 5, 2021

pgrzesik Jan 6, 2021 •

edited

Loading

medikoo Jan 7, 2021

pgrzesik Jan 7, 2021

medikoo Jan 5, 2021

medikoo left a comment

		absoluteArtifactFilePath = path.join(process.cwd(), 'newArtifact.zip');
		await fs.promises.writeFile(absoluteArtifactFilePath, zipContent);

fix(Packaging): Consider absolute artifact paths (#8325) #8315

fix(Packaging): Consider absolute artifact paths (#8325) #8315

Conversation

rib commented Oct 1, 2020 • edited Loading

codecov-commenter commented Oct 1, 2020 • edited by codecov bot Loading

Codecov Report

rib commented Oct 1, 2020

medikoo left a comment

Choose a reason for hiding this comment

rib commented Oct 1, 2020

medikoo commented Oct 1, 2020

rib commented Oct 1, 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

rib commented Oct 2, 2020

medikoo commented Oct 5, 2020

medikoo commented Oct 29, 2020

pgrzesik commented Dec 20, 2020

rib commented Dec 20, 2020

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

pgrzesik commented Jan 5, 2021

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

pgrzesik Jan 6, 2021 • edited Loading

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

rib commented Oct 1, 2020 •

edited

Loading

codecov-commenter commented Oct 1, 2020 •

edited by codecov bot

Loading

pgrzesik Jan 6, 2021 •

edited

Loading