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

[MRG] Pipfile for repo2Docker #447

Merged
merged 10 commits into from
Nov 12, 2018

Conversation

trallard
Copy link
Contributor

@trallard trallard commented Oct 18, 2018

Description

WIP: this PR should allow for repo2docker to use Pipfiles

Updated checklist:

  • My code follows the style guidelines of this project
  • I have made corresponding changes to the documentation
  • I have made corresponding changes to the contributing guidelines
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes

@betatim
Copy link
Member

betatim commented Oct 19, 2018

Whoop! Getting this done will be cool.

Just to double check: so far this PR (still work in progress) adds a Pipfile to the root of the repo which let's us install repo2docker via pipenv. The linked issue is about being able to build repos that specify their dependencies via a Pipfile. Is that where this PR is going?

@trallard
Copy link
Contributor Author

trallard commented Oct 19, 2018

Hi yes! The intention of the PR is to enable the use of Pipfiles for dependencies specification while using repo2docker to dockerize repositories (sorry for the redundancy).

I added the Pipfile for my personal use (as I use now pipenv for my dev environments).
I can leave this out of the final PR once this is ready to merge or leave it if anyone believes this will help people/contributors in any way .

@minrk
Copy link
Member

minrk commented Oct 19, 2018

Awesome! I'm excited to get this feature.

@betatim
Copy link
Member

betatim commented Oct 19, 2018

I think the biggest (and only) downside to having a Pipfile for setting up repo2docker is that we have two places (?) to keep up to date for specifying dependencies but that seems like a not so big problem. And probably something that can be solved with some automation?? -> I'd leave it in (or if the Pipfile-for-repo2docker is ready we merge this PR now and make a new one for repo2docker-learns-how-to-read-pipfiles?

@choldgraf
Copy link
Member

woo! I agree this will be a great feature. @trallard you are giving @consideRatio a challenge for "well-structured and formatted PR top-comments" :-)

@trallard
Copy link
Contributor Author

think the biggest (and only) downside to having a Pipfile for setting up repo2docker is that we have two places (?) to keep up to date for specifying dependencies but that seems like a not so big problem. And probably something that can be solved with some automation??

This is always something I worry about. A way around it would be to keep the Pipfile updated and upon any update to the dependencies generate a requirements.txt via pipenv.

Also, I this PR is on hold until perhaps next week(ish) since I will be attending conferences in the upcoming days

@choldgraf
Copy link
Member

I'd be +1 on programmatically generating the requirements.txt if we're worried that we'll forget to update one or the other (IMO the potential future value of pipfiles makes it worth it though!)

@choldgraf
Copy link
Member

hey @trallard - just a friendly ping on this one!

@trallard
Copy link
Contributor Author

trallard commented Nov 3, 2018

Oh sorry I thought I had replied. I have been away for conferences and will only be able to work on this next week.

It will work on both PRs as suggested by @betatim.
Would it then be best to have the Profile as the record of truth and then programmatically generate the requirements.txt programmatically? i e. as part of the CI/CD

@choldgraf
Copy link
Member

I'd be +1 on that...but maybe not necessary for this PR? In the short-term we can remember to update two sets of dependencies (perhaps you could add some language about the pipfile here: https://repo2docker.readthedocs.io/en/latest/contributing/tasks.html#update-and-freeze-buildpack-dependencies ?)

also, no need to say sorry! just a ping :-)

@betatim
Copy link
Member

betatim commented Nov 5, 2018

For simplicity I'd say let's go with no automation (less likely to break and need maintenance). It doesn't seem to be a common task to update the things repo2docker itself depends on. Maybe add a comment in the two places pointing to the other place.

@trallard trallard changed the title [WIP] Support Pipfiles [WIP] Pipfile for repo2Docker Nov 11, 2018
@trallard trallard changed the title [WIP] Pipfile for repo2Docker [MRG] Pipfile for repo2Docker Nov 11, 2018
Pipfile Outdated Show resolved Hide resolved
dev-requirements.txt Outdated Show resolved Hide resolved
@betatim
Copy link
Member

betatim commented Nov 11, 2018

LGTM once we resolve that one question about yaml.

Should the Pipfile <-> requirements file cross linking also mention https://github.com/jupyter/repo2docker/blob/master/docs/doc-requirements.txt as the Pipfile takes care of that as well?

Copy link
Member

@betatim betatim left a comment

Choose a reason for hiding this comment

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

If travis is happy I am happy.

Co-Authored-By: trallard <taniar.allard@gmail.com>
@trallard trallard closed this Nov 11, 2018
@trallard trallard deleted the feature/support-pipfiles-#174 branch November 11, 2018 15:56
@trallard trallard restored the feature/support-pipfiles-#174 branch November 11, 2018 15:58
@trallard trallard reopened this Nov 11, 2018
@trallard
Copy link
Contributor Author

@betatim this should fix the issues with Travis CI and I have now added the missing refs to the doc-requirements.txt too

dev-requirements.txt Outdated Show resolved Hide resolved

This should install both the dev and docs requirements at once!

### Set up
Copy link
Member

Choose a reason for hiding this comment

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

I think we need one fewer header level here (or one later header level after)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I think this heading could be completely removed since we already have a Setting up for Local Development

@choldgraf
Copy link
Member

The docs look good! I am a little bit confused: looking at the "files changed" I can't see any of the python modules being changed anymore...am I being silly and missing something here?

@betatim
Copy link
Member

betatim commented Nov 11, 2018

@choldgraf we decided to split this PR into two:

  1. adds a Pipfile so you can use pipenv to install repo2docker (this PR)
  2. a new PR that adds support to repo2docker so that it understands Pipfiles

dev-requirements.txt Outdated Show resolved Hide resolved
docs/doc-requirements.txt Outdated Show resolved Hide resolved
@choldgraf
Copy link
Member

ahhh I see, sorry for the noise :-)

betatim and others added 2 commits November 12, 2018 08:24
Co-Authored-By: trallard <taniar.allard@gmail.com>
Co-Authored-By: trallard <taniar.allard@gmail.com>
@betatim betatim merged commit 9f081a2 into jupyterhub:master Nov 12, 2018
@betatim
Copy link
Member

betatim commented Nov 12, 2018

Done and dusted! Thanks for this and looking forward to the next PR :)

@consideRatio
Copy link
Member

Wohooo nice work! :D

@betatim
Copy link
Member

betatim commented Dec 10, 2018

@trallard does your agenda have room for working on Pipfile support for creating images with repo2docker?

I'd like to make a release in a week but at some point said we'd wait for having Pipfile support. Now I am thinking we should get the release out (so that we build up some cadence on releasing) and don't wait (as there will be a next release soon after). How does that work for you?

@betatim betatim mentioned this pull request Dec 10, 2018
@trallard
Copy link
Contributor Author

heyo! I was meant to work on this last Friday but got sidetracked with the workshop applications

I have time this Friday and weekend to work on the PR but I wonder if this would be enough to work on the code and you folks to review the request.
Also, I do not want to hold this release either so perhaps can delay the Pipfile support for the upcoming release if this is not too far away

@betatim
Copy link
Member

betatim commented Dec 10, 2018

Whatever is the least amount of stress. I'd make the release now, then you add the code and we make a new release in January or so? mybinder.org runs master of repo2docker so even without a release things get out :)

@trallard
Copy link
Contributor Author

Fab! let's make this the plan and I will anyway make sure to make as much progress as possible over this weekend 😄

@betatim
Copy link
Member

betatim commented Dec 10, 2018

Sure thing, or do something away from the glowing rectangle that is a computer :)

@choldgraf
Copy link
Member

🎉

@betatim betatim mentioned this pull request Dec 22, 2018
markmo pushed a commit to markmo/repo2docker that referenced this pull request Jan 22, 2021
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.

5 participants