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

fix: Remove outdated Pipenv requirements flag #780

Merged
merged 5 commits into from
Aug 17, 2023

Conversation

jfgordon2
Copy link
Contributor

@jfgordon2 jfgordon2 commented Jul 11, 2023

Removes use of the now-removed flag --keep-outdated on pipenv lock. See pipenv 2023.7.9 release notes.

Rather than re-generating or potentially updating a user's Pipenv.lock file, it'll go straight into trying to build the requirements file.

The pre-existing behavior was that if the Pipfile.lock file was missing, that the plugin will generate one. Since this is likely undesireable, added a warning log message pointing a user to the Pipenv documenation on how to address it.

Also updates github-actions test matrix to include the before/after of this pipenv behavior change.

Closes: #779

@rwestergren
Copy link
Contributor

Thanks for fixing this - I starting running into it today.

I forked your branch and ran the Validate GH action: https://github.com/rwestergren/serverless-python-requirements/actions/runs/5579741623/jobs/10195698425

Looks like the prettier check will fail.

@jfgordon2
Copy link
Contributor Author

Thanks for fixing this - I starting running into it today.

I forked your branch and ran the Validate GH action: https://github.com/rwestergren/serverless-python-requirements/actions/runs/5579741623/jobs/10195698425

Looks like the prettier check will fail.

Ah! Good catch! I ran the prettify:updated command and pushed. Thanks @rwestergren!

@jfgordon2
Copy link
Contributor Author

@rwestergren I ran the validate workflow in my account to fix the remaining issues on commitlint, and things are now passing. I appreciate the heads-up.

@jfgordon2 jfgordon2 changed the title Pipenv requirements bugfix fix: Remove outdated Pipenv requirements flag Jul 20, 2023
Copy link
Collaborator

@pgrzesik pgrzesik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @jfgordon2 and sorry for not looking into it sooner. I have one question before merging. Do you think we shouldn't keep support for the older pipenv versions that still have the --keep-outdated flag? This is potentially a breaking change, even if the behavior might not be desirable by most people. If we drop the flag altogether, it would be great to update the docs and release it as a part of a new major version of the plugin

@jfgordon2
Copy link
Contributor Author

@pgrzesik I appreciate you taking a look!

My understanding is that pipenv lock --keep-outdated simply generated a Pipfile.lock file based on what's installed in the virtual environment but without updating to the latest versions allowed by the Pipfile definitions. To even have such an environment setup, the dev will have needed to have run pipenv sync which depends on the existence of the Pipenv.lock file or pipenv install which generates a Pipfile.lock or updates an existing one.

To me, it seems like the pipenv lock --keep-outdated is a wasted step on any version where pipenv requirements exists, and it feels like it was only kept here because the old command pattern used to be pipenv lock --requirements --keep-outdated in order to generate the requirements.txt file, and somebody was adhering to the Chesterton's fence principle in implementing the new way.

In this implementation, I'm following the Pipenv project's expectations that there will be a Pipfile.lock existing alongside the Pipfile, though added a warning just in case we don't see the expected files, and pointing a user to the Pipenv docs. We will see two potential benefits:

  1. The current pipenv lock --keep-outdated command outputs the below error, indicating to me that we could potentially have been packaging slightly different versions than what is described by the lock file. Hopefully this change makes packaging more predictable.

"The flag --keep-outdated has been deprecated for removal. The flag does not respect package resolver results and leads to inconsistent lock files. Consider using the new pipenv upgrade command to selectively upgrade packages"

  1. Removing the pipenv lock --keep-outdated operation prior to running pipenv requirements will also increase the speed of packaging with the same or possibly more predictable results.

The pipenv requirements command was implemented in v2022.4.8, and this file branches behavior on versions <=2022.8.5, so perhaps we're already supporting the older pipenv versions, anyway?

@jfgordon2 jfgordon2 requested a review from pgrzesik August 2, 2023 17:20
Copy link
Collaborator

@pgrzesik pgrzesik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarification and your work on this one. 🙇

@pgrzesik pgrzesik merged commit ad40278 into serverless:master Aug 17, 2023
24 checks passed
@jfgordon2 jfgordon2 deleted the pipenv-requirements-bug branch August 17, 2023 22:12
@andidev andidev mentioned this pull request Aug 28, 2023
3 tasks
mLupine added a commit to mLupine/serverless-python-requirements that referenced this pull request Nov 3, 2023
* master:
  Release v6.0.1 (serverless#793)
  ci: Temporarily disable test run on integrate (serverless#800)
  ci: Temporarily minimize testing matrix (serverless#799)
  ci: Fix test skips (serverless#797)
  ci: Temp skip of cache-related tests (serverless#796)
  ci: Remove node12 from testing matrix (serverless#795)
  fix: Not crash when runtime is not `python` (serverless#773)
  fix: Remove outdated Pipenv requirements flag (serverless#780)
  fix: Fix integration test matrix configuration (serverless#755)
  fix: Add legacy `pipenv` backward compatability (serverless#742)
  Add support for specifying custom dependency groups in Poetry (serverless#746)
  docs: Add contributing and code of conduct

# Conflicts:
#	test.js
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

Successfully merging this pull request may close these issues.

Pipenv --keep-outdated flag no longer supported
6 participants