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

add the COPY_HOME option in start.sh #1637

Closed
wants to merge 1 commit into from
Closed

add the COPY_HOME option in start.sh #1637

wants to merge 1 commit into from

Conversation

djangoliv
Copy link

@djangoliv djangoliv commented Feb 25, 2022

These changes make it possible to force the copy of the files of jovyan user to NB_USER

With kubespawner and user who is not jovyan, volumes mounted in /home/${NB_USER} prevent copying home jovyan files.

With kubespawner and a configuration like bellow:

c.KubeSpawner.volume_mounts = [
    {
        'name': 'common',
        'mountPath': '/home/{username}/files',
    },
]

Kubespwner create /home/{username} to mount the declared volume.
So when we execute start.sh the directory /home/{username} already exist and there is no copy of jovyan files.

With this PR, the copy can now be force with the COPY_HOME environment variable.

Regards

@consideRatio
Copy link
Collaborator

This feature has been raised for discussion already in jupyterhub/kubespawner#574 (comment). I wrote the following that I think remains relevant.

This is a discussion related to jupyter/docker-stacks. My understanding is that you discuss the following feature summarized.

  1. You have ensured there is a /home/{username}/some_dir ahead of time
  2. As part of the start.sh script of jupyter/docker-stacks runs, you have made it switch from a jovyan user to {username}. As part of this script running, you want to be able to make start.sh change its default behavior when the {username} home folderexists. You want either to replace the {username} folder if it exists, or in some way merge content from jovyan home folder to the existing {username} folder.

I would overall say that this is a quite uncommon action, and could easily lead to loss of data if misunderstood by someone. Due to that, I'm not so happy about the idea of supporting a feature like that - if I understood it correctly.


Oh... I couldn't transfer this issue to jupyter/docker-stacks as it was outside the organization. Anyhow, that is where this belongs. You could perhaps copy paste things from this to there if you want to raise it for discussion? Please if so, also include my response.


I don't think this feature should be implemented as I think it isn't an intuitive enough user experience, and I believe it could lead to confusion and loss of data - besides adding to a long term maintenance complexity.

@djangoliv
Copy link
Author

@consideRatio Sorry for the noise on kubespawner issues.

It is obviously not up to kubespawner to handle this use case.

But in docker-stacks this feature is already implemented in start.sh . This PR is just a fix for a special case.

I agree that renaming the user and their files may seem dangerous, but keeping the username in jupyterhub is very comfortable.

Copy link
Member

@mathbunnyru mathbunnyru left a comment

Choose a reason for hiding this comment

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

I'm not an expert on how start.sh works in different cases and I agree with @consideRatio's opinion.

base-notebook/start.sh Outdated Show resolved Hide resolved
base-notebook/start.sh Outdated Show resolved Hide resolved
docs/using/troubleshooting.md Outdated Show resolved Hide resolved
@consideRatio
Copy link
Collaborator

I want to avoid mixed signals of both reviewing with code suggestions and saying the feature is problematic to accept, so i'll close this PR for now.

Not because this feature doesn't have value at least to you, but because of what was mentioned above.

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.

4 participants