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

Add optional pipenv install stage #4254

Closed
wants to merge 3 commits into from

Conversation

Tobotimus
Copy link

@Tobotimus Tobotimus commented Jun 17, 2018

Hi!

I have a patch here for #3181. I went into some detail of my thought process on that issue, so I will do a quick summary of what I've done in this patch:

  • A new pipenv section in the top level of the YAML configuration, with keys enabled (bool) and options (list of strings). Here is an example:
    pipenv:
        enabled: True
        options:
        - "--dev"
        - "--ignore-pipfile"
  • If pipenv.enabled is True, pipenv==2018.5.18 (the current latest version) will be installed along with the core requirements
  • If pipenv.enabled is True, whilst setting up the python environment, after installing the user requirements and before installing the package, the pipenv install --system {pipenv.options} command will be run.

Notes

  • I have a patch ready for the readthedocs-build repository to go along with this one, should the green light be given here.
  • As of right now, I have not written documentation for this feature, I wanted to get some feedback on this implementation first 😃

Thank you maintainers for the work you put into this wonderful service ❤️

@Tobotimus Tobotimus mentioned this pull request Jun 19, 2018
Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

Thanks for the patch! I'm excited to get pipenv support merged. I'll follow up on the #3181, as there is already some discussion there.

As per conversation there, I'm going to set this to blocking as we discuss the changes there a little more. Also, we're blocked on the port of readthedocs_build to this code base, so we don't complicate the porting.

@agjohnson agjohnson added Needed: design decision A core team decision is required Status: blocked Issue is blocked on another issue labels Jun 19, 2018
@agjohnson agjohnson added this to the YAML File Completion milestone Jun 19, 2018
@agjohnson
Copy link
Contributor

So we've discuessed this more in #4354, I think we're aiming for a different schema that ties together a few more pieces. I think much of this should translate, but the schema will change fairly heavily.

@Tobotimus are you still looking to wrap up this PR? If not, @stsewd can continue this PR and put together the new schema chagnes.

@Tobotimus
Copy link
Author

@agjohnson Unfortunately these days my schedule's a bit too tight to finish this up right now, so you're welcome to do what you will 😃 Obviously these commits can be included in a subsequent PR if so desired.

@stsewd
Copy link
Member

stsewd commented Oct 6, 2018

@Tobotimus thank you! Sorry we came up with a decision too late.

@ericholscher
Copy link
Member

Closing in favor of #4783 -- thanks for the contributions here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needed: design decision A core team decision is required Status: blocked Issue is blocked on another issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants