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

fix script crash when user is not jovyan, and fix spurious warnings when $HOME environment is not "/home/jovyan" #1649

Closed
wants to merge 2 commits into from

Conversation

mayswind
Copy link

Hi, I used to build my own docker image based on the jupyter/base-notebook image with a modified username (via usermod command) previously. When I updated base-notebook to the latest version, I found that the jupyterlab failed to start without any error message.
I check the start.sh script, and I find id -u jovyan 2>/dev/null and id -g jovyan 2>/dev/null added in #1546 would return a non-zero value and then the start.sh would exit when jovyan user or jovyan group don't exist. So I define an additional function to call id command to avoid the main process geting a non-zero value.
In addition, I find there would be a warning message after I modify the $HOME environment variable. I find the code annotations describe checking the permission of the $HOME path, but the code uses a specified home path /home/jovyan. I don't know whether it is a mistake, and I also modify these code. Please correct me if my understanding is wrong.

Comment on lines +56 to +64
get_user_id () {
id -u "${1}" 2>/dev/null
return 0
}

get_group_id () {
id -g "${1}" 2>/dev/null
return 0
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, doesn't this always return 0, no matter what?

Note that when I look at this PR, I don't yet understand the situation you are in etc so I'm struggling with the review to conclude if it would make sense.

From a technical standpoint without understanding the intent of the changes, the function name above doesn't seem to do what its name indicate. It seems to include a call to the id command, but ignore its output - so then I'd wonder why even make the call in the first place for example.

I hope its okay that I ask you to open an issue to focus on what you perceive as a bug as a first priority. If it is a bug, the second step would be on how to fix it. Right now, I'm not yet understanding the bug, but figure there is some issue with an implementation to fix it - but the implementation won't be relevant before we can agree on a bug exists and should be fixed.

Due to that, I'll go for a close and hope you feel alright with opening an issue to help us reach agreement there is a bug to fix first.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you have multiple separate bugs or things you suggest we change, it is probably helpful if they are separate github issues focused one specific change.

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.

2 participants