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

finish removing files from $HOME #388

Merged
merged 2 commits into from
Jun 4, 2017
Merged

Conversation

minrk
Copy link
Member

@minrk minrk commented May 20, 2017

follow-up to #386

  • mplimporthook should be obsolete with matplotlib 2.0
  • curlrc seems to be obsolete now (updates to debian base image?)
  • set default WORKDIR to $HOME instead of $HOME/work (leave work there for
    compatibility)

There are still a couple of artefacts in $HOME, but nothing that would suffer from being wiped out due to $HOME being mounted as a fresh volume.

cc @yuvipanda

closes #331

RUN mkdir /home/$NB_USER/work && \
echo "cacert=/etc/ssl/certs/ca-certificates.crt" > /home/$NB_USER/.curlrc
# Setup work directory for backward-compatibility
RUN mkdir /home/$NB_USER/work
Copy link
Member

Choose a reason for hiding this comment

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

Is there anything else in docker-stacks using this at this point or is this of concern for some known downstream use cases?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there anything else in docker-stacks using this

No, this directory is deliberately created and left empty, so that it might be mounted as a volume. Nothing is done with it in docker-stacks.

is this of concern for some known downstream use cases?

juptyerhub-deploy-docker uses this by default, but since it mounts it as a volume, it probably wouldn't be affected. We can possibly get away with removing the directory without harming anything I know of, at least.

Copy link
Member

Choose a reason for hiding this comment

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

~/work is configured as the default notebook-dir both so that it could be mounted without disturbing all of the files in the home directory and to avoid cluttering the notebook dashboard page.

@akhmerov
Copy link
Member

Would it make sense to make $NB_USER configurable at this stage?

echo "push!(Libdl.DL_LOAD_PATH, \"$CONDA_DIR/lib\")" >> /usr/etc/julia/juliarc.jl && \
# Create JULIA_PKGDIR \
mkdir $JULIA_PKGDIR && \
chown -R $NB_USER:users $JULIA_PKGDIR
Copy link
Member

Choose a reason for hiding this comment

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

Also need to add a chown of this directory to the start-notebook.sh script as well in order to support NB_USER and (as of #387) NB_GID at docker run time. Look for the similar chown for the conda root directory.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 done. #387 removed the chown of ., which means that $HOME won't have the right permissions in this case, will it? Should I put that back, as well?

Copy link
Member

Choose a reason for hiding this comment

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

@minrk I missed your comment because GitHub folded the diff after your update to the PR. Yes, I think the . was removed by mistake and I missed it in code review. I'll send a follow-on with it.

Copy link
Member

Choose a reason for hiding this comment

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

That . used to resolve to $HOME/work. Long ago, we chmod'ed all of $HOME instead until someone pointed out that when $HOME is a host-mounted user home directory, the logic in the startup script could potentially wind up breaking ownership on the host-mounted files if $NB_USER is set poorly. I don't really see around this, however, if we want ownership set correctly from the container perspective.

- mplimporthook should be obsolete with matplotlib 2.0
- curlrc seems to be obsolete now (updates to base image?)
- set WORKDIR to $HOME instead of $HOME/work (leave work there for compatibility)
@minrk minrk force-pushed the nothing-in-home branch 2 times, most recently from c283fb1 to 276c10d Compare May 28, 2017 01:34
@minrk minrk force-pushed the nothing-in-home branch from 276c10d to 877a95d Compare May 28, 2017 01:35
@yuvipanda
Copy link
Contributor

Is this mergeable now?

@parente parente merged commit 2eee78f into jupyter:master Jun 4, 2017
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.

Consider not writing in user home
5 participants