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

Support .binder directory #653

Merged
merged 8 commits into from Apr 30, 2019

Conversation

@jhamman
Copy link
Contributor

jhamman commented Apr 28, 2019

closes #617

jhamman added 2 commits Apr 27, 2019
@betatim

This comment has been minimized.

Copy link
Member

betatim commented Apr 28, 2019

Nice work. Can you add to the docs an explicit statement that you can't "stack" these directories: use either .binder/ or binder/ and which one takes precedence over the other if you have two. I am not sure which one should take precedence, my hunch would be to make binder/ "win" because ... dunno.

@jhamman

This comment has been minimized.

Copy link
Contributor Author

jhamman commented Apr 29, 2019

I have a few tests failing here but its not clear to me why. If someone has any ideas, I'm keep to keep pushing on this.

@betatim

This comment has been minimized.

Copy link
Member

betatim commented Apr 29, 2019

tests/{venv,conda}/binder-dir/ check that the Dockerfile in the root directory of the repo is ignored if there is a binder/ directory. The venv tests on Python 3.4 also fail because of this and the dockerfile/ tests as well.

My guess is the nix related tests fail for a similar reason.

As soon as there is either a .binder/ or binder/ directory files in the root should not be found any more. By creating a specific directory you ask repo2docker to ignore everything in the top level. This let's people who have all sorts of files that repo2docker recognises in their root and ignore them when they add repo2docker support.

Despite my earlier comment about specifying the order of precedence, I now think that for consistency we should also make .binder/ and binder/ exclusive. My preference would be to make it an error to have both in your repository. That way we don't have to worry whether they should be additive or not. What do you think?

To add a test that check if things are working take a look at the test s in unit that setup temporary directories/repos and use just a small bit of repo2docker. Instead of the much heavier tests in venv/ and friends that are failing now. SOme of them could/should be implemented as unit tests with better errors and faster runtimes. So for new tests e should try and create unit tests.

repo2docker/buildpacks/base.py Outdated Show resolved Hide resolved
repo2docker/buildpacks/base.py Outdated Show resolved Hide resolved
repo2docker/buildpacks/base.py Outdated Show resolved Hide resolved
jhamman added 2 commits Apr 29, 2019
if [ -f ./binder/start ]; then
chmod u+x ./binder/start
if [ -z $binder_dir ]; then
nixpath="$binder_dir/default.nix";

This comment has been minimized.

Copy link
@betatim

betatim Apr 29, 2019

Member

From the failing nix tests it looks like it ends up setting nixpath to /default.nix and then erroring out. Not quite sure why it happens though :-/

@betatim

This comment has been minimized.

Copy link
Member

betatim commented Apr 29, 2019

After fixing up the failing nix tests, this PR needs some simple unit style tests for the functionality added and then we should be on our way to merge city 🚂 .

@jhamman

This comment has been minimized.

Copy link
Contributor Author

jhamman commented Apr 30, 2019

@betatim - unit tests added. All green!

@betatim betatim merged commit 5e67e3c into jupyter:master Apr 30, 2019
5 checks passed
5 checks passed
ci/circleci: build_docs Your tests passed on CircleCI!
Details
ci/dockercloud Your tests passed in Docker Cloud
Details
codecov/patch 100% of diff hit (target 20%)
Details
codecov/project 90.8% (+0.08%) compared to bc9554e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@betatim

This comment has been minimized.

Copy link
Member

betatim commented Apr 30, 2019

Merged! Thanks for making this happen.

@betatim

This comment has been minimized.

Copy link
Member

betatim commented Apr 30, 2019

Can you make another PR to update https://github.com/jupyter/repo2docker/blob/master/docs/source/changelog.rst with a note about this?

@consideRatio

This comment has been minimized.

Copy link
Collaborator

consideRatio commented Apr 30, 2019

Wohoooo @jhamman !!! :D 🎉 I appreciate this feature!

@jhamman jhamman referenced this pull request Apr 30, 2019
@choldgraf

This comment has been minimized.

Copy link
Collaborator

choldgraf commented May 1, 2019

indeed this is awesome! thanks @jhamman :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.