Overwrite existing requirements.txt file when generating from poetry

[tpansino]

Fixes #393 (I think)

[tpansino]

I also just realized one of the log messages from #390 that I submitted is really misleading. It removes -e flags, not lines, so I've tucked a reword for that into this PR. I hope that's ok.

[tpansino]

@dschep Any thoughts on merging this?

[lephuongbg]

@dschep Please help review and merge this fix. Without it, deploying function only (sls deploy -f function-name) won't work.

[tpansino]

@dschep welcome back, I see you're active again. 😉 Any new chance of shipping this? Or are there any another maintainers who might be able to give it a review?

[tpansino]

@bsamuel-ui I see you going through cleaning up old issues. Can this be merged as a follow up item from #390 ? That PR doesn't currently fix the issue for me without these changes.

[miketheman]

@tpansino Thanks for your continued attention to this.

Recently #446 was merged - this adopted the poetry post-1.0.0 release option of export with -o flag as the default behavior now, and likely resolves the underlying issue - can you please confirm if this is still a problem?

[lephuongbg]

I can confirm that this is still a problem with 5.1 version. I have to add 'rm -f .serverless/requirements.txt' to run before function packaging using serverless-scriptable-plugins.

To reproduce, first make a normal deploy. See that the .serverless folder is there. Then do a single function deploy, at which it will fail because current code aren't allowed to overwrite file.

[tpansino]

I haven't tested, but I can also confirm just by looking at the latest release that the issue isn't resolved yet. The overwrite I've added here is necessary as @lephuongbg has said.

[miketheman]

To reproduce, first make a normal deploy. See that the .serverless folder is there. Then do a single function deploy, at which it will fail because current code aren't allowed to overwrite file.

Thanks! this was the reproducing steps I needed.

Things I've learned:

sls package works fine. sls deploy && sls deploy -f hello shows the error:

Serverless: Load command param:list
Serverless: Invoke deploy
Serverless: Invoke deploy:function
Serverless: [AWS lambda 200 0.104s 0 retries] getFunction({ FunctionName: 'sls-py-req-test-dev-hello' })
Serverless: Generating requirements.txt from pyproject.toml...
Serverless: The generated file contains -e lines, removing them...

  Error --------------------------------------------------

  Error: EEXIST: file already exists, link '/Users/miketheman/workspace/miketheman/serverless-python-requirements/tests/poetry/requirements.txt' -> '/Users/miketheman/workspace/miketheman/serverless-python-requirements/tests/poetry/.serverless/requirements.txt'
      at Object.linkSync (fs.js:938:3)
      at tryRenameSync (/Users/miketheman/workspace/miketheman/serverless-python-requirements/tests/poetry/node_modules/fs-extra/lib/move-sync/index.js:40:12)
      at Object.moveSync (/Users/miketheman/workspace/miketheman/serverless-python-requirements/tests/poetry/node_modules/fs-extra/lib/move-sync/index.js:22:3)
      at ServerlessPythonRequirements.pyprojectTomlToRequirements (/Users/miketheman/workspace/miketheman/serverless-python-requirements/tests/poetry/node_modules/serverless-python-requirements/lib/poetry.js:62:7)

After further digging, I've found this to be true - that there's lifecycleEvents in the aws plugin in the core framework that runs when performing package and deploy, and for deploy, only when not specifying the -f <function name>).

These can be seen with SLS_DEBUG=* sls deploy:

...
Serverless: Invoke deploy
Serverless: Invoke package
Serverless: Invoke aws:common:validate
Serverless: Invoke aws:common:cleanupTempDir
Serverless: Generating requirements.txt from pyproject.toml...
...

vs SLS_DEBUG=* sls deploy -f hello:

...
Serverless: Invoke deploy
Serverless: Invoke deploy:function
Serverless: [AWS lambda 200 0.12s 0 retries] getFunction({ FunctionName: 'sls-py-req-test-dev-hello' })
Serverless: Generating requirements.txt from pyproject.toml...
...

We can see that the package step is not invoked, since it's already been packaged, and the aws:common:cleanupTempDir lifecycle event is not run.

That lifecycle event is where the core AWS plugin is removing the .serverless directory - code is here: https://github.com/serverless/serverless/blob/f93b27bf684d9a14b1e67ec554a7719ca3628135/lib/plugins/aws/common/lib/cleanupTempDir.js

So to recap, here's what the problem is:

• Certain types of commands clear out the .serverless directory - but not all do.
• the moveSync function won't overwrite an existing file unless told to, and the file may already exist when invoking the sls deploy -f <function> command, since the aws plugin hasn't cleaned up the directory
• the pipenv behavior is different - as it uses writeFileSync instead of moveSync

So there's a few of paths forward here that I can see:

1. Accept this PR to enable moveSync to overwrite an existing file with a freshly-exported one
2. Convert the file-writing behavior to be more aligned with the pipenv approach using writeFileSync without the need to move it after writing. Potentially look at extracting that as common functionality, since there's definitely some overlap there.
3. Something else?

I'm leaning towards the first, and adding a backlog issue to do the second later.

Curious to know what @bsamuel-ui and @AndrewFarley might think about this, as well as any other ideas.

[miketheman]

Upon further testing, I tried adding the suggested override and it still fails - largely due to the input of a function not having a package artifact.
Before proceeding, I'd need a better example of a failing serverless.yml and function structure:

...
Serverless: Invoke package:function
Serverless: Packaging function: hello...
Serverless: Excluding development dependencies...

  Type Error ---------------------------------------------

  TypeError: Cannot read property 'artifact' of undefined
      at ServerlessPythonRequirements.BbPromise.bind.then.then.then (/Users/miketheman/workspace/miketheman/serverless-python-requirements/index.js:176:48)
  From previous event:
      at PluginManager.invoke (/usr/local/lib/node_modules/serverless/lib/classes/PluginManager.js:490:22)
      at PluginManager.spawn (/usr/local/lib/node_modules/serverless/lib/classes/PluginManager.js:510:17)
      at Deploy.BbPromise.bind.then (/usr/local/lib/node_modules/serverless/lib/plugins/deploy/deploy.js:110:50)
...

[AndrewFarley]

@miketheman This PR doesn't look like it would break anything, so I would give it a thumbs-up as a possible preventative measure just for the sake of consistency.

[miketheman]

based on the current behaviors, I'm approving this. @tpansino can you please rebase against current master branch to run it against the most recent test suite?

[lephuongbg]

Upon further testing, I tried adding the suggested override and it still fails - largely due to the input of a function not having a package artifact.

@miketheman That was another bug: serverless/serverless#6752. You could add an empty package: {} to the function config as in comment-589507064 which will bypass that bug for now.

[tpansino]

@miketheman Sorry, I somehow didn't get notified of your approval.

I merged master but the build died on ci:lint with what looks like a Node trace. Is this a known issue?

[piohhmy]

Thanks for submitting this @tpansino ! @miketheman can this get included into next release?