fix: Adapt to support latest `pipenv` version #718

andidev · 2022-09-03T15:06:39Z

As mentioned in issue #716 pipenv lock --requirements --keep-outdated command no longer works and throws an error since --requirements flag has been deprecated and is now removed in latest version of pipenv (versions larger then or equal to 2022.8.13).
In this fix pipenv requirements --hash is now called after pipenv lock --keep-outdated instead of passing the removed --requirements flag.

Tested to serverless deploy with this fix and it works.

FelipeBarrosCruz · 2022-09-07T02:30:47Z

would be awesome if this PR could be reviewed fast.
Thank you for your work @andidev .

woozyking · 2022-09-07T19:21:40Z

lib/pipenv.js

@@ -32,7 +32,14 @@ async function pipfileToRequirements() {
    try {
      res = await spawn(
        'pipenv',
-        ['lock', '--requirements', '--keep-outdated'],
+        ['lock', '--keep-outdated'],


does it make sense to retain this command spawn if its result is anyway overridden by the new requirements --hash command?

pipenv lock command
generates Pipfile.lock file
https://pipenv.pypa.io/en/latest/cli/#pipenv-lock

pipenv requirements command
requires a Pipfile.lock file to run
https://pipenv.pypa.io/en/latest/cli/#pipenv-requirements

but I guess when running this plugin the Pipfile.lock is already there on place so it should be ok to remove as you say. but I am a bit to new to python and this plugin to be 100% sure to be honest.

i see. so perhaps it's needed if some repo lacks the lock file?

This is a needed fix. With the deprecated --requirements param I get this error:
Error: pipenv lock --requirements --keep-outdated Exited with code 2

I removed this parameter on my local code and works.

fair enough!

toxygene · 2022-09-26T14:08:31Z

Any update on this issue? I am impacted by this issue and it looks like the PR is languishing while waiting for a review.

pgrzesik · 2022-09-27T17:38:14Z

Thanks a lot, @andidev - it looks like there are some failing tests, but I believe it's a problem with the whole test suite in general. I'll try to stabilize it in the coming days and we can come back to this PR

pgrzesik · 2022-09-29T21:36:28Z

Hey @andidev 👋 I think the tests should be pretty stable now, do you think you could rebase against the current main branch? Thanks in advance 🙇

andidev · 2022-09-30T02:05:47Z

@pgrzesik done!

pgrzesik · 2022-09-30T22:23:54Z

Hey @andidev, thanks a lot. It looks like the tests on CI are failing, could you please look into that?

andidev · 2022-10-01T02:14:46Z

@pgrzesik yeah so it was some prettier stuff not formatted.
would recommend to add githooks to autoformat on commit.

pgrzesik · 2022-10-01T07:49:34Z

Hey @andidev - it failed on prettier in the first job, but other too also failed with tests that failed, please see: https://github.com/serverless/serverless-python-requirements/actions/runs/3155560666/jobs/5138325172

As for autoformatting on commit - that's a good suggestion for an improvement 👍

rwestergren · 2022-10-01T13:11:40Z

Looks like pipenv is pinned at version 2021.11.5 for the tests:

andidev · 2022-10-01T16:45:03Z

@pgrzesik so I updated to latest version of pipenv in tests.
I aslo created a PR for configuring to run eslint and prettier on a pre-commit hook, see PR #731

rwestergren · 2022-10-01T20:01:28Z

@andidev Created a PR for your fork to fix the pipenv tests - was missing a Pipfile.lock file:

Pipfile.lock must exist to use --keep-outdated!

andidev#1

Had a successful run on my fork (ignore the commit message failure): https://github.com/rwestergren/serverless-python-requirements/actions/runs/3165785814/jobs/5155070848

andidev · 2022-10-02T04:59:02Z

So it should be possible to merge now. @rwestergren thanks!

pgrzesik

Thanks a lot @andidev and @rwestergren 🙇 I have a few questions before we finalize this PR, the major one around it being a potentially breaking change.

pgrzesik · 2022-10-03T21:12:47Z

.github/workflows/integrate.yml

@@ -45,7 +45,7 @@ jobs:
        run: python -m pip install --force setuptools wheel

      - name: Install pipenv / poetry
-        run: python -m pip install pipenv==2021.11.5 poetry


Maybe at this point we could remove the pinned pipenv version here - I added it temporarily to stabilize tests and be able to merge other PRs

pgrzesik · 2022-10-03T21:13:28Z

lib/pipenv.js

-          cwd: this.servicePath,
-        }
-      );
+      res = await spawn('pipenv', ['lock', '--keep-outdated'], {


This feels to me like a change that might be potentially breaking for people on some older pipenv versions. Do you know if there's a minimal pipenv version needed for these two commands?

It's definitely breaking for some older versions as there was no requirements command. Could you please verify which version of pipenv is now required? We should specify it in docs

pgrzesik · 2022-10-03T21:14:00Z

tests/pipenv/Pipfile.lock

@@ -0,0 +1,281 @@
+{


Is there a reason to commit lock here? I think we could leave it out

Lock file is needed for the tests, otherwise Pipfile.lock must exist to use --keep-outdated!

#718 (comment)

So, that changes the previous behavior where lock file was not needed? What if we just start a new project with intention of managing the dependencies with Pipfile? Is it going to fail too on the first run?

These tests are for pipenv specifically and I'm not familiar with a scenario where lock isn't expected. To my knowledge, it should always be created at first usage.

The very basic scenario is where you are just starting out the project for the first time and you don't have Pipfile.lock generated yet.

I've just tested it locally and this PR breaks in that scenario. We need to have a way to still support situations where someone is just starting out. In old implementation, in such case, it's progressing correctly and it generates Pipfile.lock. cc @andidev

I was looking into it a bit more, and it seems like there's not good workflow at the moment for fresh projects that do not have Pipfile.lock generated yet. I think one option would be to first check for existence of Pipfile.lock, if it's avaialble, we should run the commands as proposed in this PR. If it's not present, we should skip adding the --keep-outdated flag as it crashes the lock command when there's not Pipfile.lock. What do you think?

It seems to me both commands actually require a Pipfile.lock

[pipenv-test]$ pipenv requirements --hash ... FileNotFoundError: [Errno 2] No such file or directory: '/tmp/pipenv-test/Pipfile.lock'

At this point, I personally think a lockfile should be a prerequisite for pipenv support in this project, since it generally seems to be for pipenv itself.

I agree that pipenv requirements --hash requires Pipfile.lock, as its generating requirements.txt from the lockfile.

The problem that I'm bringing up is the fact that if we accept the PR as-is, then we're breaking the functionality for users that are using the file on a new project or want to remove Pipfile.lock in order to regenerate it during deploy. In previous (current) implementation, if lockfile is not present, it is generated automatically for you. In proposed implementation, you run into an error and in order to resolve it, you need to somehow manually generate Pipfile.lock e.g. by running pipfile lock command. It seems to me like this is a way worse experience and I don't see why we wouldn't want to support generating lockfile if it doesn't exist.

What do you think @rwestergren @andidev?

evanatlas · 2022-10-17T15:34:56Z

What's the current status of this? Currently using @andidev's feature branch, merging this PR would unblock me.

pgrzesik · 2022-10-17T21:59:30Z

Hey @evanatlas, there are still some outstanding issues with the proposed implementation that are blocking finalization. @andidev Do you think you'll have time to address them in the coming days? If not, I can pick it up and finalize this PR probably by the end of the week. Let me know 💯

andidev · 2022-10-23T15:35:01Z

@pgrzesik
Sorry for late response here. No I don't. Have too much work elsewhere unfortunately. Please feel free to finalize it.

pgrzesik · 2022-10-23T15:37:28Z

Thanks, @andidev, as you can see I kinda already did, I'm planning to release a new major version of the plugin in the coming days so I wanted to include that as it will include a breaking change for people on older pipenv versions. Thanks a lot for your work on this 🙇

ronkorving · 2022-10-25T03:58:45Z

Thanks everyone for addressing this issue. I'm looking forward to the release.

woozyking reviewed Sep 7, 2022

View reviewed changes

xncbf previously approved these changes Sep 12, 2022

View reviewed changes

douglasmoraisdev previously approved these changes Sep 12, 2022

View reviewed changes

Zwirek009 previously approved these changes Sep 27, 2022

View reviewed changes

andidev dismissed stale reviews from Zwirek009, douglasmoraisdev, and xncbf via 03e2bcf September 30, 2022 00:47

andidev force-pushed the support-latest-pipenv branch from 9d15ee4 to 03e2bcf Compare September 30, 2022 00:47

andidev force-pushed the support-latest-pipenv branch from 03e2bcf to cda6faa Compare October 1, 2022 01:43

pgrzesik added the enhancement label Oct 1, 2022

andidev force-pushed the support-latest-pipenv branch from 29a0a43 to b317d6f Compare October 1, 2022 20:17

pgrzesik reviewed Oct 3, 2022

View reviewed changes

andidev and others added 3 commits October 23, 2022 17:12

Add support for latest pipenv

04d8338

Update pipenv in test to latest version 2022.9.24

955c27f

Fix pipenv tests

ef4f0cc

pgrzesik force-pushed the support-latest-pipenv branch from ce329e6 to 2f1e440 Compare October 23, 2022 15:12

pgrzesik force-pushed the support-latest-pipenv branch 2 times, most recently from 96b32ee to a9e5ef0 Compare October 23, 2022 15:33

pgrzesik force-pushed the support-latest-pipenv branch 8 times, most recently from 6f1b801 to 6576dbc Compare October 23, 2022 18:24

feat: Ensure support when Pipfile.lock missing

1766993

pgrzesik force-pushed the support-latest-pipenv branch from 6576dbc to 1766993 Compare October 23, 2022 18:39

pgrzesik changed the title ~~Add support for latest pipenv version solves #716~~ fix: Adapt to support latest pipenv version Oct 23, 2022

pgrzesik merged commit 853da8d into serverless:master Oct 23, 2022

pgrzesik approved these changes Oct 23, 2022

View reviewed changes

pgrzesik mentioned this pull request Oct 24, 2022

Plugin not compatible with =>2022.8.13 of pipenv #716

Closed

rwestergren mentioned this pull request Nov 13, 2022

fix: Add legacy pipenv backward compatability #742

Merged

swiffer mentioned this pull request Dec 27, 2022

pipenv generation of requirements.txt changed #753

Closed

3 tasks

stackspotadmin mentioned this pull request Feb 11, 2024

[Snyk] Upgrade serverless-python-requirements from 5.4.0 to 6.0.1 stack-spot/stackspot-python-stack#22

Open

brittag mentioned this pull request Jun 19, 2024

[Snyk] Upgrade serverless-python-requirements from 5.4.0 to 6.1.0 Enterprise-CMCS/cmcs-eregulations#1349

Merged

brittag mentioned this pull request Sep 26, 2024

[Snyk] Upgrade serverless-python-requirements from 5.4.0 to 6.1.1 Enterprise-CMCS/cmcs-eregulations#1430

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Adapt to support latest `pipenv` version #718

fix: Adapt to support latest `pipenv` version #718

andidev commented Sep 3, 2022

FelipeBarrosCruz commented Sep 7, 2022

woozyking Sep 7, 2022

andidev Sep 9, 2022

woozyking Sep 12, 2022

douglasmoraisdev Sep 12, 2022

woozyking Sep 26, 2022

toxygene commented Sep 26, 2022

pgrzesik commented Sep 27, 2022

pgrzesik commented Sep 29, 2022

andidev commented Sep 30, 2022

pgrzesik commented Sep 30, 2022

andidev commented Oct 1, 2022

pgrzesik commented Oct 1, 2022

rwestergren commented Oct 1, 2022

andidev commented Oct 1, 2022

rwestergren commented Oct 1, 2022

andidev commented Oct 2, 2022

pgrzesik left a comment

pgrzesik Oct 3, 2022

pgrzesik Oct 3, 2022

pgrzesik Oct 5, 2022

pgrzesik Oct 3, 2022

rwestergren Oct 3, 2022

pgrzesik Oct 3, 2022

rwestergren Oct 3, 2022

pgrzesik Oct 4, 2022

pgrzesik Oct 5, 2022

rwestergren Oct 6, 2022

pgrzesik Oct 8, 2022

evanatlas commented Oct 17, 2022

pgrzesik commented Oct 17, 2022

andidev commented Oct 23, 2022

pgrzesik commented Oct 23, 2022

ronkorving commented Oct 25, 2022

fix: Adapt to support latest pipenv version #718

fix: Adapt to support latest pipenv version #718

Conversation

andidev commented Sep 3, 2022

FelipeBarrosCruz commented Sep 7, 2022

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

toxygene commented Sep 26, 2022

pgrzesik commented Sep 27, 2022

pgrzesik commented Sep 29, 2022

andidev commented Sep 30, 2022

pgrzesik commented Sep 30, 2022

andidev commented Oct 1, 2022

pgrzesik commented Oct 1, 2022

rwestergren commented Oct 1, 2022

andidev commented Oct 1, 2022

rwestergren commented Oct 1, 2022

andidev commented Oct 2, 2022

pgrzesik 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

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

evanatlas commented Oct 17, 2022

pgrzesik commented Oct 17, 2022

andidev commented Oct 23, 2022

pgrzesik commented Oct 23, 2022

ronkorving commented Oct 25, 2022

fix: Adapt to support latest `pipenv` version #718

fix: Adapt to support latest `pipenv` version #718