-
Notifications
You must be signed in to change notification settings - Fork 689
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
Update urllib3 to 1.25.3 #4665
Update urllib3 to 1.25.3 #4665
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #4665 +/- ##
========================================
Coverage 82.67% 82.67%
========================================
Files 45 45
Lines 3122 3122
Branches 338 338
========================================
Hits 2581 2581
Misses 454 454
Partials 87 87 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rmol . Hashes look good to me , and performed diff review based on the following version bumps, looks good to me.
- requests (2.20.0->2.22.0) : https://github.com/psf/requests/compare/v2.20.0..v2.22.0
- urllib (1.24.1->1.25.3): https://github.com/urllib3/urllib3/compare/1.24.1..1.25.3
- pip-tools 3.5.0 -> 4.0.0: https://github.com/jazzband/pip-tools/compare/3.5.0..4.0.0
I will test this PR functionally and will comment/approve.
requests==2.20.0 \ | ||
--hash=sha256:99dcfdaaeb17caf6e526f32b6a7b780461512ab3f1d992187801694cba42770c \ | ||
--hash=sha256:a84b8c9ab6239b578f22d1c21d51b696dcfe004032bb80ea832398d6909d7279 | ||
requests==2.22.0 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✔️
pip-tools==3.5.0 \ | ||
--hash=sha256:0018485119986aebef136470c51bde85da736732079c687ab1d4c5eb5237e694 \ | ||
--hash=sha256:a395ca8bb32bcaea58c8da89a2518793d88b43b15152217ba117c4170e507af9 | ||
pip-tools==4.0.0 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✔️
virtualenv==15.1.0 \ | ||
--hash=sha256:02f8102c2436bb03b3ee6dede1919d1dac8a427541652e5ec95171ec8adbc93a \ | ||
--hash=sha256:39d88b533b422825d644087a21e78c45cf5af0ef7a99a1fc9fbb7b481e5c85b0 \ | ||
# via tox | ||
wrapt==1.10.11 \ | ||
--hash=sha256:d4d560d479f2c21e1b5443bbd15fe7ec4b37fe7e53d335d3b9b0a7b1226fe3c6 \ | ||
# via astroid | ||
|
||
# WARNING: The following packages were not pinned, but pip requires them to be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any idea why this warning is here? Is it due to the new version of pip-tools?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Since pip-compile
refuses to hash setuptools
unless given --allow-unsafe
, if whatever version of setuptools
was installed when the virtualenv was created needed to be upgraded to satisfy another package's dependencies, pip
would fail.
There's some discussion on that issue suggesting that it would be safer to use --allow-unsafe
to let pip-compile
pin and hash setuptools
. That makes sense to me, but that should probably be a separate PR, and maybe @kushaldas would like to weigh in here.
urllib3==1.23 \ | ||
--hash=sha256:a68ac5e15e76e7e5dd2b8f94007233e01effe3e50e8daddf69acfd81cb686baf \ | ||
--hash=sha256:b5725a0bd4ba422ab0e66e89e030c806576753ea3ee08554382c14e685d117b5 | ||
urllib3==1.25.3 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✔️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment inline regarding the discrepancy between python2/develop-requirements.txt
changes and python3/develop-requirements.txt
I've uploaded the diffs of the source tarballs reviewed as part of this pr review (the GH links were for convenience) (thanks @kushaldas for the out-of-band reminder)
I've tested the staging scenario and everything functions correctly/all tests pass, CI as also passing (converge and verify).
I will approve this (but not merge), in case anyone has objections to using different versions of pip-tools in the python2 or python3 dev environments, they can feel free to comment on this ticket. Given the python2 environment should be rapidly deprecated after the release of 1.0.0, I think this is fine since these are dev/test requirements.
@@ -71,7 +71,7 @@ pathspec==0.5.5 # via yamllint | |||
pathtools==0.1.2 # via sphinx-autobuild, watchdog | |||
pbr==5.1.1 # via git-url-parse, molecule, python-gilt, stevedore | |||
pexpect==4.6.0 # via molecule | |||
pip-tools==3.8.0 | |||
pip-tools==3.9.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why was pip-tools updated here but not in python3 develop requirements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question. Should we just standardize on pip-tools==4.0.0
everywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good to merge when CI passes
Admin also needed pip-tools==4.0.0 to work with pip>=19.2, which is added to the bootstrapped virtualenv.
f2702c8
to
53d0e7b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
restamping after rebase
Status
Work in progress
Description of Changes
Fixes #4379.
Admin also needed pip-tools==4.0.0 to work with pip>=19.2, which is added to the bootstrapped virtualenv.
Testing
Automated tests should suffice to get the new requirements installed and used.
Deployment
I think this is dev-only; it's updating requirements in the admin virtualenv (requests), but as far as I can tell they're only used in tests.
Checklist
If you made changes to the server application code:
make lint
) and tests (make -C securedrop test
) pass in the development containerIf you made changes to
securedrop-admin
:make -C admin test
) pass in the admin development container