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

Implement Pipfile and remove extra requirements.txt files #47626

Merged
merged 6 commits into from May 16, 2018

Conversation

Projects
None yet
6 participants
@gtmanfred
Contributor

gtmanfred commented May 13, 2018

What does this PR do?

Instead of having extra requirements.txt files, we can just specify only install dependencies based on which version of python we are using.

Also, add a Pipfile for pipenv, because it is super awesome, and gives us lockfiles/virtualenvs.

Based on what we saw at PyCon 2018, this will be a super awesome way to manage pip stuff and will be included in pip in the future.

https://us.pycon.org/2018/schedule/presentation/243/

Commits signed with GPG?

Yes

@gtmanfred gtmanfred requested review from terminalmage and s0undt3ch May 13, 2018

.gitignore Outdated
@@ -11,6 +11,7 @@ MANIFEST
*.wpr
*.wpu
*.DS_Store
Pipfile.lock

This comment has been minimized.

@terminalmage

terminalmage May 13, 2018

Member

The creator of pipenv recommended keeping the Pipfile and lockfile both in version control.

This comment has been minimized.

@gtmanfred

gtmanfred May 13, 2018

Contributor

Agreed, once I learned that you can still install newer stuff even if the lock file is there, it makes sense that you should put the lock file in VCS.

@gtmanfred gtmanfred force-pushed the gtmanfred:2017.7 branch from 55b4e11 to 60c3546 May 13, 2018

@gtmanfred

This comment has been minimized.

Contributor

gtmanfred commented May 13, 2018

Thanks @kennethreitz for pipenv

@andrewthetechie

This comment has been minimized.

andrewthetechie commented May 13, 2018

This is awesome! Go pipenv!

Pipfile Outdated
moto = ">=0.3.6"
SaltPyLint = ">=v2017.3.6"
pytest = ">=3.5.0"
Pytest-catchlog = {git = "git://github.com/eisensheng/pytest-catchlog.git", ref = "develop", markers = "python_version < '3.0'"}

This comment has been minimized.

@s0undt3ch

s0undt3ch May 13, 2018

Member

Drop this one, if it py.test explodes, I have a fix

This comment has been minimized.

@gtmanfred

gtmanfred May 13, 2018

Contributor

Done

moto>=0.3.6
SaltPyLint>=v2017.3.6
pytest>=3.5.0
git+https://github.com/eisensheng/pytest-catchlog.git@develop#egg=Pytest-catchlog; python_version < '3.0'

This comment has been minimized.

@s0undt3ch

s0undt3ch May 13, 2018

Member

same here

This comment has been minimized.

@gtmanfred

gtmanfred May 13, 2018

Contributor

Done

This comment has been minimized.

@gtmanfred

gtmanfred May 13, 2018

Contributor

looks like pytest still works.

This comment has been minimized.

@s0undt3ch

s0undt3ch May 13, 2018

Member

:applause:

@s0undt3ch

🎉

gtmanfred added some commits May 13, 2018

simplify dev and base.txt to single files
Instead of having different files for different python versions, just have one,
and use the built in `python_version` to handle which dependencies to install.

@gtmanfred gtmanfred force-pushed the gtmanfred:2017.7 branch from 60c3546 to e0f7cc1 May 13, 2018

@kennethreitz

This comment has been minimized.

kennethreitz commented May 13, 2018

This is awesome! 🍰

gtmanfred added some commits May 13, 2018

@rallytime rallytime requested a review from terminalmage May 15, 2018

@rallytime

This is neat! I have a couple of very picky suggestions and a question on this change.

@@ -102,6 +102,14 @@ def pytest_addoption(parser):
'SSH server on your machine. In certain environments, this '
'may be insecure! Default: False'
)
test_selection_group.addoption(

This comment has been minimized.

@rallytime

rallytime May 15, 2018

Contributor

Is this change intentional? This seems unrelated to the rest of the changes here.

This comment has been minimized.

@gtmanfred

gtmanfred May 15, 2018

Contributor

It was intentional, it fixes pytest for salt, so you can run integration tests.

I can move it to a new PR if you want.

This comment has been minimized.

@rallytime

rallytime May 15, 2018

Contributor

Nah, it's good. Just making sure.

pytest>=3.5.0
git+https://github.com/eisensheng/pytest-catchlog.git@develop#egg=Pytest-catchlog
git+https://github.com/saltstack/pytest-salt.git@master#egg=pytest-salt
# This is for legacy purposes

This comment has been minimized.

@rallytime

rallytime May 15, 2018

Contributor

I know this is picky, but it might make sense for this comment to match the one in dev_python34.txt (and I liked that one the best of the two). 😄

This comment has been minimized.

@gtmanfred

gtmanfred May 15, 2018

Contributor

Will do

moto>=0.3.6
SaltPyLint>=v2017.3.6
pytest>=3.5.0
git+https://github.com/eisensheng/pytest-catchlog.git@develop#egg=Pytest-catchlog

This comment has been minimized.

@rallytime

rallytime May 15, 2018

Contributor

I don't see the pytest-catchlog in the new dev.txt file. I am not sure what that particular addition to the Py27 requirements file is for since there's no comment about it, but I wanted to double check that this was intentional to leave that out.

This comment has been minimized.

@gtmanfred

gtmanfred May 15, 2018

Contributor

pytest-catchlog has been merged into pytest upstream, so we no longer needed it specified.

This comment has been minimized.

@rallytime

rallytime May 15, 2018

Contributor

Perfect!

@gtmanfred gtmanfred merged commit 0d06da6 into saltstack:2017.7 May 16, 2018

7 of 9 checks passed

default Build finished.
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #18981 — FAILURE
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #25111 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #17211 — SUCCESS
Details
jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #4924 — SUCCESS
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #22858 — SUCCESS
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #9894 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #21841 — SUCCESS
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment