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

Copy repo to ${REPO_DIR} rather than ${HOME} #507

Merged
merged 9 commits into from Dec 20, 2018

Conversation

@yuvipanda
Copy link
Collaborator

yuvipanda commented Dec 16, 2018

We want to put the repo somewhere other than ${HOME}
so we can mount persistent storage in ${HOME} more easily.
Most of repo2docker assumes current directory is where
the contents are, so we should be able to get most of it
working by setting WORKDIR to ${REPO_PATH} and letting
${REPO_PATH} be configurable.

Unclear what to do for plain Dockerfiles, Legacy Dockerfiles &
nix.

@yuvipanda

This comment has been minimized.

Copy link
Collaborator Author

yuvipanda commented Dec 16, 2018

We need to basically test all our integration tests with REPO_PATH set to something other than ${HOME}. That might make our test suite much slower, unfortunately. Maybe we could get away with a subset?

repo2docker/buildpacks/base.py Outdated Show resolved Hide resolved
@yuvipanda yuvipanda force-pushed the yuvipanda:path branch from c1ec3eb to 42c826a Dec 17, 2018
@yuvipanda

This comment has been minimized.

Copy link
Collaborator Author

yuvipanda commented Dec 18, 2018

We currently set PATH to include ${HOME}/.local/bin. This is the XDG standard's location for per-user installs of stuff - but maybe repositories are putting stuff there and expecting it to work? In which case it won't work when REPO_PATH is somewhere else.

One possibility is to add both ${HOME}/.local/bin and ${REPO_PATH}/.local/bin to PATH...

@yuvipanda

This comment has been minimized.

Copy link
Collaborator Author

yuvipanda commented Dec 18, 2018

Should we call it REPO_PATH or REPO_DIR?

@yuvipanda yuvipanda changed the title [WIP] Copy repo to ${REPO_DIR} rather than ${HOME} Copy repo to ${REPO_DIR} rather than ${HOME} Dec 18, 2018
@betatim

This comment has been minimized.

Copy link
Member

betatim commented Dec 18, 2018

How about REPO_DESTINATION?

We added ~/.local/bin to PATH explicitly to let people put things on the PATH from their repositories or from postBuild. I think we need to maintain that behaviour so I'd go for adding both home and the repositories .local/bin to the PATH.

@minrk

This comment has been minimized.

Copy link
Member

minrk commented Dec 18, 2018

Should we call it REPO_PATH or REPO_DIR?

A convention I like is PATH vars are lists of dirs (like $PATH) and DIR for single directories

yuvipanda added a commit to yuvipanda/repo2docker that referenced this pull request Dec 18, 2018
yuvipanda added 8 commits Dec 16, 2018
We want to put the repo somewhere other than ${HOME}
so we can mount persistent storage in ${HOME} more easily.
Most of repo2docker assumes current directory is where
the contents are, so we should be able to get most of it
working by setting WORKDIR to ${REPO_PATH} and letting
${REPO_PATH} be configurable.

Unclear what to do for plain Dockerfiles, Legacy Dockerfiles &
nix.
Leads to maximum cache re-use
Expanding env vars through ARG and ENV is too confusing
to get right.
extra-args.yaml placed in a test directory can be used
to pass extra arguments to the repo2docker command.
@yuvipanda yuvipanda force-pushed the yuvipanda:path branch from 5d880d8 to 29407aa Dec 18, 2018
@minrk

This comment has been minimized.

Copy link
Member

minrk commented Dec 19, 2018

Looks excellent to me now, thanks @yuvipanda! Do we want to place any restrictions on REPO_DIR? I could imagine someone doing interesting things with REPO_DIR=/etc. But I guess in cases where you care, it's the service operator who controls REPO_DIR, right? Not users?

@minrk
minrk approved these changes Dec 19, 2018
@yuvipanda

This comment has been minimized.

Copy link
Collaborator Author

yuvipanda commented Dec 19, 2018

@minrk yep - admins control it so we should be fine. We gotta be careful if / when we let users explicitly set environment values. But I think that is unlikely since we have start

@minrk minrk merged commit d12afc2 into jupyter:master Dec 20, 2018
4 checks passed
4 checks passed
ci/circleci: build_docs Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 20%)
Details
codecov/project 85.06% (+0.08%) compared to bdd32b2
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.