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

Pyup pulls fail when pip-compile changes whitespace #747

Closed
Zac-HD opened this issue Jul 28, 2017 · 12 comments
Closed

Pyup pulls fail when pip-compile changes whitespace #747

Zac-HD opened this issue Jul 28, 2017 · 12 comments
Assignees
Labels
tests/build/CI about testing or deployment *of* Hypothesis

Comments

@Zac-HD
Copy link
Member

Zac-HD commented Jul 28, 2017

See #738, #743, #745 for currently-open examples.

The short version is that when @pyup-bot edits the version number of a dependency, it doesn't account for the fixed-indent comment added by pip-compile. The check-requirements job therefore fails for updates that change the number of characters in a version identifier (eg 1.0.9 -> 1.0.10) because there is a diff when the spacing is restored.

Our options seem to be (a) keep fixing this manually and hope it goes away, (b) ignore whitespace in the git diff for this check and accept uneven comments, (c) ask Pyup to recognize and handle pip-compile output, or (d) use something other than Pyup for version checks (eg a cron-job running pip-compile).

Assigned to @DRMacIver as he has both the relevant credentials and final decision on this kind of policy.

Update: we have another failure mode - if A requires B in a certain range, Pyup will try to update B to the latest version regardless. Ref: #945.

@Zac-HD Zac-HD added the tests/build/CI about testing or deployment *of* Hypothesis label Jul 28, 2017
@Zac-HD
Copy link
Member Author

Zac-HD commented Aug 9, 2017

@DRMacIver, can you simply disable Pyup? It often causes conflict with the check-requirements job, and that job will ensure that everything is in order anyway whenever a PR is merged. I mean, we currently have four broken pulls by pyup and that's just silly.

@DRMacIver
Copy link
Member

I propose we just disable check-requirements job entirely and deal with the misaligned comments (does anyone ever manually look at those files anyway?). It's not buying us anything that pyup doesn't already give us. We included it on a "might as well" basis but it turns out to be a bit of a pain.

@Zac-HD
Copy link
Member Author

Zac-HD commented Aug 12, 2017

It's not buying us anything that pyup doesn't already give us.

It is, actually - dependency checks. For example, #775 is failing (here) because pytest-xdist added a dependency on pytest-forked which isn't captured in the lockfile by Pyup.

@DRMacIver
Copy link
Member

It is, actually - dependency checks. For example, #775 is failing (here) because pytest-xdist added a dependency on pytest-forked which isn't captured in the lockfile by Pyup.

Ugh, right.

OK. So here's a thing we could do: The pyup bot creates branches in this repo. We have the functionality to make changes from travis already. We could have check-requirements verify if it's on a pyup branch (easy to do: Their name starts with pyup) and if so rebuild and push the changed requirements file.

Does that sound reasonable?

@Zac-HD
Copy link
Member Author

Zac-HD commented Oct 4, 2017

The requirements check for #913 is now failing because of a Pyup/pip-compile clash, and only four days since the last instance in #905.

IMO that makes it time to implement @DRMacIver's suggestion above - if the check-requirements job fails on a branch starting with pyup-update-, commit and push to the branch.

@DRMacIver
Copy link
Member

Related, but I thought I'd changed pyup to weekly batching and it doesn't seem to have taken if so.

@Zac-HD
Copy link
Member Author

Zac-HD commented Apr 17, 2018

...having taken a closer look at the commit-and-push proposal, I don't think we can do that in a way that doesn't expose commit access to someone sneaky who opens their own pull.

This also hasn't been much of a problem since we went to weekly updates, so I suggest we just deal with it for a few minutes a month.

@Zac-HD Zac-HD closed this as completed Apr 17, 2018
@DRMacIver
Copy link
Member

...having taken a closer look at the commit-and-push proposal, I don't think we can do that in a way that doesn't expose commit access to someone sneaky who opens their own pull.

I think we can! A distinguishing feature that prevents this is that the pyup branches are on this repo rather than an external fork, so anyone who can push to such a branch already has commit access.

That being said, you and @alexwlchan are so much more on the ball than I am about picking up these pulls that it doesn't really matter to me either way, so I'm happy to leave it up to you.

@Zac-HD
Copy link
Member Author

Zac-HD commented Apr 17, 2018

TBH I think it's less maintenance effort to do it by hand than to keep it working automatically, so there we are.

The problem with checking branches is that any such protection would be defined in configuration that lives in this repo, and thus subject to change by a malicious pull. Or at least that's my understanding, and I'm not keen to get clever with security.

@greysteil
Copy link

Hey team,

I saw your issue over a Pyup, and thought I'd chime in. I built Dependabot, a similar service that already has the ability to execute arbitrary code safely. As a result, I've been able to add Pipfile support for Python pretty straightforwardly, and am about to add pip-compile support.

I'm going to use the setup in this repo as a reference point, so Dependabot will definitely work with it. Would you be up for giving it a try when it's done (should be tomorrow or over the weekend)? The service is already used by a few thousand people, including GitHub themselves.

@DRMacIver
Copy link
Member

Would you be up for giving it a try when it's done (should be tomorrow or over the weekend)?

I'm certainly keen to give it a go. Especially given the current problems we're having where pyup's requirements updating completely ignores version compatibility issues and we have to manually run pip-compile each week ourselves anyway!

@greysteil
Copy link

Awesome! I just shipped support and ran it on my fork - looks like everything's working as expected so you should be good to go if you want to give it a try.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests/build/CI about testing or deployment *of* Hypothesis
Projects
None yet
Development

No branches or pull requests

3 participants