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

Remove upper bound on pip version #991

Merged
merged 3 commits into from
Dec 17, 2018

Conversation

stealthycoin
Copy link
Contributor

@stealthycoin stealthycoin commented Nov 22, 2018

Since pip no longer uses semver the upper bound isn't particularly
useful. It also prevents Chalice from using newer versions once pip is
upgraded. Chalice will also downgrade the environment's pip version when
installed which can be a source of annoyance.

fixes #981

Since pip no longer uses semver the upper bound isn't particularly
useful. It also prevents Chalice from using newer versions once pip is
upgraded. Chalice will also downgrade the environment's pip version when
installed which can be a source of annoyance.
@codecov-io
Copy link

codecov-io commented Nov 22, 2018

Codecov Report

Merging #991 into master will decrease coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #991      +/-   ##
==========================================
- Coverage   95.52%   95.47%   -0.06%     
==========================================
  Files          27       27              
  Lines        4449     4487      +38     
  Branches      558      563       +5     
==========================================
+ Hits         4250     4284      +34     
- Misses        128      131       +3     
- Partials       71       72       +1
Impacted Files Coverage Δ
chalice/config.py 95.48% <0%> (-2.53%) ⬇️
chalice/compat.py 40% <0%> (-0.55%) ⬇️
chalice/cli/factory.py 92.85% <0%> (-0.05%) ⬇️
chalice/deploy/swagger.py 100% <0%> (ø) ⬆️
chalice/app.py 96.76% <0%> (+0.01%) ⬆️
chalice/deploy/packager.py 95.88% <0%> (+0.16%) ⬆️
chalice/cli/__init__.py 87.4% <0%> (+0.2%) ⬆️

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 84e6b53...eeea8e3. Read the comment docs.

@jamesls
Copy link
Member

jamesls commented Nov 28, 2018

I'm hesitant to have no upper bound on this, especially given we rely on internal pip APIs. Can we just lock it to a year?

@jamesls jamesls self-assigned this Nov 29, 2018
@jamesls
Copy link
Member

jamesls commented Nov 29, 2018

@stealthycoin I pushed a change to lock to <19. My concern is mostly about future changes to pip that won't be compatible with chalice. If a user has a working app on a certain version of chalice, then it's possible that chalice may not deploy correctly in the future because there's no upper bound on pip.

@dstufft
Copy link

dstufft commented Nov 29, 2018

Fundamentally there's nothing different from the 18.X -> 19.0 release than there is for the 18.X -> 18.X+1 release. It seems wrong to me to lock it to a year for the stated reason, because locking it to a year doesn't actually solve the problem of that a release of pip can break chalice. We should probably either keep the ceiling as is, or remove it.

@jamesls
Copy link
Member

jamesls commented Dec 4, 2018

@dstufft The main thing I'd like to prevent is automatically using new versions of pip until we've confirmed that it passes through our CI successfully. With no ceiling, we don't have any recourse if a future version of pip breaks our usage of it other than to cut a new release and ask customers to immediately upgrade (or require them to track which version of pip works with what version of chalice).

My intent was to lock to the latest version of pip, so we can be more explicit with <=18.1. It does mean we have to keep on top of pip releases every 3 months.

Long term, perhaps we should look at what it would take to vendor the specific parts of pip that we use. Open to other ideas.

@jamesls
Copy link
Member

jamesls commented Dec 6, 2018

Quick update, I changed the dependency to <=18.1. This is essentially what we've been doing so far, which is to lock to a version of pip (or less) that we know works with chalice.

Yes, we'll have to keep this updated with new pip releases, but I think the longer term plan should to investigate vendoring this code into chalice.

@jamesls jamesls merged commit 5c0cad5 into aws:master Dec 17, 2018
@dmulter
Copy link
Contributor

dmulter commented Feb 19, 2019

This is a problem again now that pip 19.0.2 is out.

@duaneking
Copy link

This defect is exactly why this project seems so dead; You guys are testing the wrong things and this arbitrary limit is overly restrictive and creates more work every time PIP is updated, as well as greatly harms usage of the library because we as developers CANT TRUST IT when we try to install it the first time and find it actively conflicts with our other pip and package installs.

Pip installs should never break packages; there is a very valid reason for venvs and that is why you should use them... but best practice in python is always to use the newest version of pip so there is no excuse for this.

A version of PIP getting upgraded shouldn't even have the possibility of breaking your modules if they are coded correctly; What are you doing that could break based on pip? It seems to me that is the problem here, remove the sickness of having an upper limit on pip and this sickness of not being able to use chalice for a month or two every time pip updates goes away.

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.

Incompatible with Pip>18.0
8 participants