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

Revise requirements files again #982

Merged
merged 1 commit into from
Feb 19, 2020

Conversation

lukpueh
Copy link
Member

@lukpueh lukpueh commented Feb 18, 2020

Fixes issue #:
#978 follow-up

Description of the changes being introduced by the pull request:
Follows up on #978, which had the following problems:

  • too many requirements files (cc @trishankatdatadog ;)
  • used extra tooling around pip-compile that
    • didn't take into account requirement markers (see comments in requirements.txt in this commit), and
    • confused Dependabot, which expects the hashed requirements file in a certain format, as pip-compile would generate it without custom tooling (see build(deps): bump cffi from 1.13.2 to 1.14.0 #979).

This commit restructures the requirements files as follows:

  • Merges requirements-tox.txt and requirements-test.txt. The separation was semantically correct but operationally irrelevant.
  • Removes the hashed requirements file, which doesn't add much security, especially with PEP 458 on the way (see PEP 458: Mark as Accepted python/peps#1306), but extra maintenance (see notes about requirements.txt in Revise requirements files and remove pyup #978 and about Dependabot above)
  • Manually adds environment markers to requirements-pinned.txt (see comments in requirements.txt in this commit)

Please verify and check that the pull request fulfills the following
requirements
:

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

@lukpueh
Copy link
Member Author

lukpueh commented Feb 18, 2020

@joshuagl, would you mind taking a quick look at this PR?

Follows up on theupdateframework#978, which had the following problems:
- too many requirements files (cc @trishankatdatadog ;)
- used extra tooling around pip-compile that
  - didn't take into account requirement markers (see comments
    in requirements.txt in this commit), and
  - confused Dependabot, which expects the hashed requirements
    file in a certain format, as pip-compile would generate it
    without custom tooling (see theupdateframework#979).

This commit restructures the requirements files as follows:

- Merges requirements-tox.txt and requirements-test.txt. The
  separation was semantically correct but operationally irrelevant.
- Removes the hashed requirements file, which doesn't add much
  security, especially with PEP 458 on the way (see python/peps#1306),
  but extra maintenance (see notes about requirements.txt in theupdateframework#978
  and about Dependabot above)
- Manually adds environment markers to requirements-pinned.txt (see
  comments in requirements.txt in this commit).

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
lukpueh added a commit to lukpueh/securesystemslib that referenced this pull request Feb 18, 2020
TODO:
- Add Dependabot badge
- Compare to theupdateframework/python-tuf#982 and in-toto/in-toto#294
- Grep for renames
- Create commits
@joshuagl
Copy link
Member

This looks like a reasonable set of changes to me.

It's not 100% clear to me from the message what about the prior requirements.txt was causing dependabot to choke, but I don't think that should block this merge. We seem to have equivalent functionality, modulo hashes in the requirements.txt, which is a small price to pay for our dependencies being automatically monitored and updated.

@lukpueh
Copy link
Member Author

lukpueh commented Feb 19, 2020

Many thanks for the review, @joshuagl! You're right, my comments above don't shine much light on the why of the problem. That's mostly because I'm not so sure about it. One would probably have to delve into Dependabot's ruby scripts, which I don't want to. (The main reason for switching to Dependabot, was that I didn't want to troubleshoot Pyup.)

From playing around a little bit, I saw that Dependabot has troubles updating a requirements.txt with hashes, if a requirements.in file exists, but wasn't used to generate requirements.txt with the vanilla pip-compile --generate-hashes command. Interestingly, Dependabot does update the same file, if there is no requirements.in (apparently there are other problems with the hashed requirements files too, see e.g. dependabot/dependabot-core#1587).

Given that we require some custom and manual tooling around pip-compile (see comments in requirements.txt), I decided to remove the hashed file, so Dependabot doesn't choke with the hashes, and we only have to half-manually maintain one requirements file (i.e. the pinned one).

Also, while the purpose of the pinned requirements file seem quite convincing, that is auto-triggering tests against all latest dependencies whenever any of them is updated, the benefits of hashed requirements do less, at least in regards to the issues described above.

I hope this clarifies my motivation.

@joshuagl
Copy link
Member

Thanks for taking the time to write that up @lukpueh. I agree with your motivation – switching to a more reliable dependency monitoring/updating tool.

LGTM.

@lukpueh lukpueh merged commit 42f4c3f into theupdateframework:develop Feb 19, 2020
@lukpueh lukpueh mentioned this pull request Oct 22, 2020
3 tasks
lukpueh added a commit to lukpueh/in-toto that referenced this pull request Dec 11, 2020
Adopt docs for updating requirements-pinned.txt from based on
their revision in tuf and sslib:
- theupdateframework/python-tuf#982
- secure-systems-lab/securesystemslib#209

The update includes a transfer of the doc header + script/commands
from requirements-pinned.txt to requirements.txt, and a thus
resulting simplification of the commands.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
lukpueh added a commit to lukpueh/in-toto that referenced this pull request Dec 11, 2020
Adopt docs for updating requirements-pinned.txt from based on
their revision in tuf and sslib:
- theupdateframework/python-tuf#982
- secure-systems-lab/securesystemslib#209

The update includes a transfer of the doc header + script/commands
from requirements-pinned.txt to requirements.txt, and a thus
resulting simplification of the commands.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
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.

None yet

2 participants