Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Zip file resolution error when deploying a single artifact #82

Open
tstackhouse opened this issue May 26, 2021 · 18 comments
Open

Comments

@tstackhouse
Copy link
Collaborator

tstackhouse commented May 26, 2021

Describe the bug
When overwriting servicePath with the packagePath in deploy.impl.ts:123 (and in earlier iterations) because of the relative pathing, inside serverless' packaging routines, the package path gets appended twice, resulting in a silent failure caused by an ENOENT when deploy attempts to read the generated zip artifacts:

https://github.com/serverless/serverless/blob/master/lib/plugins/package/lib/zipService.js#L67
https://github.com/serverless/serverless/blob/master/lib/plugins/aws/deploy/lib/checkForChanges.js#L158-L162

package.artifact is populated when package.individually is set to false in serverless.yml:
https://github.com/serverless/serverless/blob/master/lib/plugins/package/lib/packageService.js#L99-L113

Inspecting the filePath that comes back in packageAll, we can see the following:

{
  zipFileName: 'my-app.zip',
  filePath: 'dist/.serverlessPackages/apps/my-app/.serverless/my-app.zip'
}

That filePath then gets set to service.package.artifact which we can see in checkIfDeploymentIsNecessary in checkForChanges.js we can inspect zipFiles and zipFilePaths and see the incorrect path resolution for artifact:

{
  artifact: 'dist/.serverlessPackages/apps/my-app/.serverless/my-app.zip',
  zipFiles: [
    'my-app.zip',
    '/home/my-user/my-repo/dist/.serverlessPackages/apps/my-app/dist/.serverlessPackages/apps/my-app/.serverless/my-app.zip'
  ],
  zipFilePaths: [
    '/home/my-user/my-repo/dist/.serverlessPackages/apps/my-app/.serverless/my-app.zip',
    '/home/my-user/my-repo/dist/.serverlessPackages/apps/my-app/dist/.serverlessPackages/apps/my-app/.serverless/my-app.zip'
  ]
}

We can see that the resolution on the prior step appends servicePath again which then on subsequent lines results in a silent ENOENT when serverless attempts to calculate the hash of the non-existant zip file. I already have tried to reproduce this through serverless directly, but as we are manipulating the runtime configuration in real time here, it is not easily reproducible through the command line for sls.

Suggested resolution
After a lot of trial and error if we modify the servicePath override on https://github.com/flowaccount/nx-plugins/blob/master/libs/nx-serverless/src/builders/deploy/deploy.impl.ts#L123 to be like so:

        ServerlessWrapper.serverless.config.servicePath = path.resolve(packagePath);

This solves the path resolution issue by preventing path.resolve from incorrectly attempting to append the path:

{
  artifact: '/home/my-user/my-repo/dist/.serverlessPackages/apps/my-app/.serverless/my-app.zip',
  zipFiles: [
    'my-app.zip',
    '/home/my-user/my-repo/dist/.serverlessPackages/apps/my-app/.serverless/my-app.zip'
  ],
  zipFilePaths: [
    '/home/my-user/my-repo/dist/.serverlessPackages/apps/my-app/.serverless/my-app.zip',
    '/home/my-user/my-repo/dist/.serverlessPackages/apps/my-app/.serverless/my-app.zip'
  ]
}

I want to submit a PR, but I've already spent days on this, and would need to upgrade my fork to match the master branch which includes the beta work for supporting serverless 2, which is a whole other can of worms for my project.

My repo with the change is here: https://github.com/tstackhouse/nx-plugins along with a solo build branch so I can pull it into my project: https://github.com/tstackhouse/nx-plugins/tree/nx-serverless-2

I have it on my radar to plan for upgrades in the near future (I hope) and am excited to see this flourish. I've seen some of the work to support Nx11, which would be awesome as well.

Check which provider is affected:
[x] AWS
[?] Azure
[?] Google Cloud Platform

Check which framework is affected:
[ ] Angular
[x] Nodejs
[x] Serverless
[ ] Lambda
[ ] Infrastructure as a code

Additional context
Currently using 0.5.3 of this plugin
Node 12.18.3
npm 6.14.6
serverless:
Framework Core: 1.79.0
Plugin: 3.7.1
SDK: 2.3.1
Components: 2.34.6

@wickstargazer
Copy link
Member

Hey i just saw this bug. Is it happening to everyone who is trying to use the beta version then?

@wickstargazer
Copy link
Member

@tstackhouse would this help? --> https://github.com/flowaccount/nx-plugins/pull/84/files#diff-83dc8d582b8fb37ed63b28f9d66778aaf1b91d48495c4085387fda2eed4afdef

also i merged the latest code into master although the test isn't working yet. Will try to find time to make them work.
but if this helps will release beta.4

@tstackhouse
Copy link
Collaborator Author

It's a step in the right direction, though I was doing some more testing and found another issue in the upload artifacts step when not packaging individually: https://github.com/serverless/serverless/blob/master/lib/plugins/aws/deploy/lib/uploadArtifacts.js#L104-L116

That usage of packagePath there is the culprit, because that gets set way that the initialization of the library here: https://github.com/serverless/serverless/blob/master/lib/plugins/aws/deploy/index.js#L22-L25

And because of that, it's not actually overridden by the time we do the override in deploy.impl.ts. Using package.individually avoids going into that if statement and sidesteps the issue.

@wickstargazer
Copy link
Member

so should we set the package.artifact?
and test it out to see if it works.
To summarize a bit. It works if not packaging individually correct? since i tested out and that seems ok.

@tstackhouse
Copy link
Collaborator Author

The opposite actually, at least in my experience. Packaging individually works because it goes inside both if statements in that code in uploadArtifacts.js.

Not packaging individually only goes into the outer if, as the artifact is set in https://github.com/serverless/serverless/blob/master/lib/plugins/package/lib/packageService.js#L99-L113 during the packat step.

Commenting the return on line 115 allows the if statement to be exited and the deploy works correctly.

@wickstargazer
Copy link
Member

Commenting the return on line 115 allows the if statement to be exited and the deploy works correctly. in which file?

@wickstargazer
Copy link
Member

wickstargazer commented Jun 4, 2021

problem is i released 1.0.0-beta and the code has changed Alot....my bad for lack of communications. Want to make sure it works and we are talking about the same issue. I am testing out with packagingIndividually flags now

@wickstargazer
Copy link
Member

wickstargazer commented Jun 4, 2021

ok i found out that if i set package.individually: false the error of the zip file occurs. I am on the right track right?

There was an error with the build. Error: ENOENT: no such file or directory, open
dist\.serverlessPackages\apps\api\lambda\dist\.serverlessPackages\apps\api\lambda.user-analytics\.serverless\lambda-.zip

trying to solve it

@wickstargazer
Copy link
Member

i traced it down to something in serverless itself

image

the servicePath is changed and not same as serviceDir in

serverless\lib\plugins\aws\deploy\index.js. In constructor it is set to ServiceDir but along the way its changed! ..... line 97 i log it and the value has changed.

still figuring out

@tstackhouse
Copy link
Collaborator Author

tstackhouse commented Jun 4, 2021

Commenting the return on line 115 allows the if statement to be exited and the deploy works correctly. in which file?

Sorry, I was on my phone and getting out a detailed reply is tricky. like 115 in uploadArtifacts.js: https://github.com/serverless/serverless/blob/master/lib/plugins/aws/deploy/lib/uploadArtifacts.js#L115

problem is i released 1.0.0-beta and the code has changed Alot....my bad for lack of communications. Want to make sure it works and we are talking about the same issue. I am testing out with packagingIndividually flags now

No worries about the 1.0.0-beta, I need to see what if anything I would need to do to use serverless 2 in my project over 1.x.

ok i found out that if i set package.individually: false the error of the zip file occurs. I am on the right track right?

There was an error with the build. Error: ENOENT: no such file or directory, open
dist.serverlessPackages\apps\api\lambda\dist.serverlessPackages\apps\api\lambda.user-analytics.serverless\lambda-.zip

trying to solve it

That's exactly it. I did see that you fixed an issue in the beta that I was going to submit a fix for that I saw in 0.5.3:

In the beta:

throw new Error(`There was an error with the build. ${ex}.`);

in 0.5.3: https://github.com/flowaccount/nx-plugins/blob/0.5.3/libs/nx-serverless/src/builders/deploy/deploy.impl.ts#L138-L141

There was no logging on deploy failure, so it would silently exit the nx command and the only indicator was an exit code of 1. I had added a log call so help me trace the failures in my debugging: https://github.com/tstackhouse/nx-plugins/blob/master/libs/nx-serverless/src/builders/deploy/deploy.impl.ts#L141-L147

@wickstargazer
Copy link
Member

yeh I had to refactor the whole thing to work with nx-12 .. Its another pattern that is used with nx/dev-kit and generators etc etc as well.

So i am investigating why the path changed. I think it has something to do with the serverless lifeCycle hooks
not sure which hook changed the variable.

@tstackhouse
Copy link
Collaborator Author

I'm investigating updating to 2.x and at least upon first impressions, the way this plugin handling initializing serverless, if we use an SSM reference in our serverless.yml, the framework attempts to parse the config and fails because the aws plugin isn't loaded, I tried commenting out all my SSM params from the config, but when it gets to the deploy step, it seems to blow out:


npx nx deploy my-api --stage=my-stage

// Build output removed

dist/.serverlessPackages/apps/my-api
Copying build output files from dist/apps/my-api to dist/.serverlessPackages/apps/my-api to be packaged
Done copying build output files.
running serverless commands
There was an error with the build. ServerlessError: Serverless command "deploy" not found. Did you mean "undefined"? Run "serverless help" for a list of all available commands..

@wickstargazer
Copy link
Member

wickstargazer commented Jun 4, 2021 via email

@tstackhouse
Copy link
Collaborator Author

tstackhouse commented Jun 4, 2021

Researching a bit more, it almost seems like something is causing the AWS plugins to not load at all:

Serverless: Load command param:list
Serverless: Load command studio
Serverless: Configuration warning: Unrecognized provider 'aws'
Serverless:  
Serverless: You're relying on provider plugin which doesn't provide a validation schema for its config.

--
Continuing to dig around, it looks like the sls builder doesn't include compat in the 1.0.0-beta.3 dist:
image
image

10086$ npx nx run my-api:sls help
> nx run my-api:sls 
Cannot find module '/home/me/my-project/node_modules/@flowaccount/nx-serverless/src/builders/sls/compat'
Require stack:
- /home/me/my-project/node_modules/@angular-devkit/architect/node/node-modules-architect-host.js
...

--
More realtime debugging research: It looks like deploy might not be initializing serverless before attempting to run any commands (in your serverless.js file), build.impl.js doesn't yet use your refactored library. I'm still picking through things, though it still seems like the AWS plugin isn't being loaded... It looks like this could be something with my environment, but I'm not sure. The plugin has it's own node_modules and subsequently, it's own serverless. when I build, the project level Serverless gets called and initialized, but then when deploy is called, it's calling the serverless inside the plugin, so none of the commands are there and hence the error I saw in my prior comment. I'm not sure how best to fix it yet :/

@wickstargazer
Copy link
Member

wickstargazer commented Jun 7, 2021

in serverless.ts you need to change the first import to something like

import * as Serverless from '<path to your debugging-workspace>/node_modules/serverless/lib/Serverless.js'; instead of 'serverless/lib/Serverless';

this will make thigs work

@wickstargazer
Copy link
Member

@tstackhouse the deploy per function is working now in 1.0.0-beta.8 can we close this issue?
its fixed in this PR #97

@tstackhouse
Copy link
Collaborator Author

I've been able to get back into this a bi, trying to update to 1.1.0 and serverless 2 in my project, and this bug is related to a slightly different issue, when using packageIndividually: false in serverless.yml a single zip file is built to be deployed, however due to the hotpatching necessary to support the .serverlessPackages paths, this file resolution fails. It also seems like my proposed solution in the original issue, using path.resolve() prior to the hotpatch on the serverless object doesn't seem to work properly with serverless 2+.

I'm kind of just putting words on paper at this point, so forgive me, I've been iterating on this and getting back into the headspace all day today. It seems to me that it should be possible through configuration or code changes to alleviate this, I keep coming back to the .serverlessPackages piece being one potential thread that contributes to this. Is there a reason to have this path over just packaging directly in dist?

@tstackhouse
Copy link
Collaborator Author

I've actually tracked down an easy potential fix for this, by changing the code here: https://github.com/flowaccount/nx-plugins/blob/master/libs/nx-serverless/src/utils/serverless.ts#L126-L132 from:

      if (
        deployOptions &&
        deployOptions.function &&
        deployOptions.function != ''
      ) {
        serverlessConfig.servicePath = getPackagePath(deployOptions);
      }

to

      if (deployOptions) {
        serverlessConfig.serviceDir = getPackagePath(deployOptions);
      }

Omitting the extra conditionals and correcting the config option name a project with pacakgeIndividually: false in serverless.yml allows for a single zip file deployment to go through properly by setting the config value earlier in the initialization process and avoiding the need to hotpatch the config.

I can try to pull together a PR tomorrow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants