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

Make requirements with pip-tools 3.8.0 #24514

Merged
merged 5 commits into from
Jun 7, 2019
Merged

Make requirements with pip-tools 3.8.0 #24514

merged 5 commits into from
Jun 7, 2019

Conversation

millerdev
Copy link
Contributor

Attempt to fix travis build failure seen in various PRs (ex: #24509).

@emord

@millerdev millerdev added the product/invisible Change has no end-user visible impact label Jun 7, 2019
Copy link
Contributor

@emord emord left a comment

Choose a reason for hiding this comment

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

Very confused by this PR.

  1. requests[security] was changed to just requests which made a lot of changes to the requirements on prod.
  2. pip-tools is pinned to 3.6.0. To upgrade it to 3.8.0 dev-requirements.in should have chaned and make upgrade-requirements should have been run.
  3. when i upgraded to 3.8.0 locally I do see the added setup tools, but I don't see all of these other changes that have been made

@millerdev
Copy link
Contributor Author

millerdev commented Jun 7, 2019

So one problem I had with the original version of this PR is that I ran make requirements with an active Python 3 virtualenv. That's fixed now (force-pushed).

Also, I think this is another problem:

pip install pip-tools

Installs the latest version of pip-tools, not the version pinned in dev-requirements.in

@@ -33,7 +33,7 @@ commonmark==0.9.0 # via recommonmark
concurrent-log-handler==0.9.12
contextlib2==0.5.5
coverage==4.5.1
cryptography==2.6.1
cryptography==2.7
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused how this was upgraded. make requirements is supposed to be strictly making sure that the requirements fit in with current definition in *.in files. It should only change when running make upgrade-requirements or changing the in files. (it didn't show up in #24519)

Copy link
Contributor

@emord emord left a comment

Choose a reason for hiding this comment

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

My comment isn't immediately actionable. just an idea. I think this PR is still a better place than it was before

@@ -1,7 +1,7 @@
#!/usr/bin/env bash
set -e

pip install pip-tools==3.6.1
pip install pip-tools>=3.8
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that this is still fragile to this kind of change. Long term, maybe we should run the requirements test after the other tests? that way this comparison could run in the same virtualenv where we have pinned pip-tools.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Long term, maybe we should run the requirements test after the other tests?

Sure, I'll do that in a separate PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@emord emord merged commit b7057b5 into master Jun 7, 2019
@emord emord deleted the dm/make-reqs branch June 7, 2019 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/invisible Change has no end-user visible impact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants