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

Removes top level requirements.* #42

Merged
merged 1 commit into from
Sep 12, 2017

Conversation

kurtwheeler
Copy link
Contributor

@kurtwheeler kurtwheeler commented Aug 23, 2017

This will resolve: #17

These two files are not actually required by anything in this project. In the past I think there were some dependencies which were needed for the whole project but all of these got moved into the Docker containers for different sub-projects. The only python package needed by the entire repo is pip-tools, but the install.sh script installs that into a virtualenv for the user, so it doesn't seem necessary to have a requirements.txt with only that one line in it.

Copy link

@mhuyck mhuyck left a comment

Choose a reason for hiding this comment

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

This seems like a good cleanup to me. Just two things to make sure you've thought about:

(1) I assume pep8 and flake8 are things you'd want for development still, but maybe you don't need them integrated in to the source repo. Do you have another way to automate running these tools?

(2) One potential flaw with installing pip-tools via shell script the way you have is that you no longer control for the release version. This could conceivably make reproducibility hard. Have you considered this angle?

@kurtwheeler
Copy link
Contributor Author

Re:

Do you have another way to automate running these tools?
I'm not entirely sure what you mean there. I have them installed on my machine, but I guess I don't have a way to install easily other than running the commands to install them (optionally into a virtualenv).

Re:

This could conceivably make reproducibility hard.
I don't think that will be an issue because the only thing pip-tools is used for is to compile requirements.ins into requirements.txts. Since both of these files are under source control I don't see how reproducible would be hard, since all the requirements.txt files specify the exact version for everything that is actually installed into a Docker container.

@mhuyck
Copy link

mhuyck commented Aug 24, 2017

Re: pep8 and flake8:

Sorry, I sorta conflated the step of installing these tools with how they are used. To elaborate: we don't have pep8 or flake8 in our repo for adage-server either, so it makes sense to me to remove them here. Each of us still uses them (or a similar linter) to check code, though. I have a trigger in my editor to run them for Python files. We also rely on codeclimate integration through GitHub to perform those checks and more.

@mhuyck
Copy link

mhuyck commented Aug 24, 2017

Re: pip-tools:

Good point. Tracking the specific version of that tool does not matter as much as what it's actually installing. Mostly I just wanted to make sure you had considered the implications of that. It also occurs to me that you'll have reproducibility somewhat covered by the Docker repository as well, right? If you needed to get back to a specific container state you should be able to dive into the history of the images you've pushed.

@kurtwheeler kurtwheeler merged commit 886ee15 into master Sep 12, 2017
@kurtwheeler kurtwheeler deleted the cleanup-top-level-requirements branch September 12, 2017 18:27
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.

2 participants