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

Make sure ENTRYPOINT is an absolute path #657

Merged
merged 1 commit into from Apr 30, 2019

Conversation

@yuvipanda
Copy link
Collaborator

yuvipanda commented Apr 30, 2019

Unlike other parts of the generated Dockerfile,
the start script is evaluated at run time, rather
than at build time. Currently, we assume that the
current working directory is the same at runtime
as build time for the start script. This doesn't
hold true always, and particularly not in JupyterHub
environments where ${HOME} is often overlaid with
a persistent directory.

We change this to always refer to the full path,
using the ${REPO_DIR} environment variable. This
lets people building JupyterHub images to set
REPO_DIR to something like /srv/repo (like hubploy
does), and still have a working start script.

@betatim

This comment has been minimized.

Copy link
Member

betatim commented Apr 30, 2019

LGTM. Can you add a quick test with a temporary directory (unit-test style) to make sure this remains an absolute path and respects REPO_DIR also in the future?

@yuvipanda

This comment has been minimized.

Copy link
Collaborator Author

yuvipanda commented Apr 30, 2019

@betatim yeah, I can do that. I'm actually tempted to instead pull out just the ENTRYPOINT related stuff from #651 instead.

@minrk minrk closed this Apr 30, 2019
@minrk minrk reopened this Apr 30, 2019
@betatim betatim referenced this pull request Apr 30, 2019
5 of 5 tasks complete
Unlike other parts of the generated Dockerfile,
the start script is evaluated at run time, rather
than at build time. Currently, we assume that the
current working directory is the same at runtime
as build time for the start script. This doesn't
hold true always, and particularly not in JupyterHub
environments where ${HOME} is often overlaid with
a persistent directory.

We change this to always refer to the full path,
using the ${REPO_DIR} environment variable. This
lets people building JupyterHub images to set
REPO_DIR to something like /srv/repo (like hubploy
does), and still have a working start script.
@yuvipanda yuvipanda force-pushed the yuvipanda:abs-start branch from cd1e450 to 4dd32f3 Apr 30, 2019
@yuvipanda

This comment has been minimized.

Copy link
Collaborator Author

yuvipanda commented Apr 30, 2019

@betatim I rebased this, but it's ready to go now (thanks to #651 being merged)

@yuvipanda yuvipanda referenced this pull request Apr 30, 2019
5 of 13 tasks complete
@betatim betatim merged commit e976627 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%) compared to 5e67e3c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
yuvipanda added a commit to yuvipanda/datahub that referenced this pull request Apr 30, 2019
Get in jupyter/repo2docker#657
so we can use 'start' with w261 image.
# a working directory that is different from ${REPO_DIR}
# This isn't a problem with anything else, since start is
# the only path evaluated at container start time rather than build time
return os.path.join('${REPO_DIR}', start)

This comment has been minimized.

Copy link
@consideRatio

consideRatio May 1, 2019

Collaborator

Okay so we join a placeholder path to get started, then we should not add any quotes!

image

This comment has been minimized.

Copy link
@betatim

betatim May 1, 2019

Member

What do you mean?

This comment has been minimized.

Copy link
@consideRatio

consideRatio May 1, 2019

Collaborator

Sorry for confusion, often quotes are required in paths for bash to interpret it all as one command line argument, this as it should be, no additional quotes would make sense

@consideRatio

This comment has been minimized.

Copy link
Collaborator

consideRatio commented May 1, 2019

LGTM!!

Woops post merge ;D

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.