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

Revert broken sed --in-place command #1521

Merged
merged 1 commit into from
Nov 10, 2021

Conversation

maresb
Copy link
Contributor

@maresb maresb commented Nov 10, 2021

Partial fix for #1520

base-notebook/start.sh Outdated Show resolved Hide resolved
@maresb
Copy link
Contributor Author

maresb commented Nov 10, 2021

@mathbunnyru I'm usually not so aggressive about force-pushing, but I just can't bring myself to make a commit to remove a blank line. :)

@mathbunnyru
Copy link
Member

That's fine :)

@maresb
Copy link
Contributor Author

maresb commented Nov 10, 2021

We also have the extraneous warnings from #1520... I'm wondering what's the best way to fix those.

# /etc/ and we may not have write access. Apply sed on our own temp file:
sed --expression="s/^jovyan:/nayvoj:/" /etc/passwd > /tmp/passwd
echo "jovyan:x:$(id -u):$(id -g):,,,:/home/jovyan:/bin/bash" >> /tmp/passwd
cat /tmp/passwd > /etc/passwd
Copy link
Member

Choose a reason for hiding this comment

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

That's quite a funny situation when we can't create a file in /etc, but can completely overwrite an existing one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya... so /etc/passwd has g+w and jovyan is in the root group.

base-notebook/start.sh Outdated Show resolved Hide resolved
@maresb
Copy link
Contributor Author

maresb commented Nov 10, 2021

I think we pulled the trigger too early on that PR. Should have written tests first. Oh well.

I think we should leave the false warnings in there for now. I don't have time to deal with them right away. I find the code before the PR more logical in this part.

@mathbunnyru
Copy link
Member

@maresb yeah, our testing is not the best, there are too many cases and shell scripts are not tested easily.

So, what do you propose?
I can merge this one quickly, you checked, that this fixes the issue, right?
I would appreciate if you added a test for this later.

@maresb
Copy link
Contributor Author

maresb commented Nov 10, 2021

Ya, let's do a quick merge here. We'll work on test cases and warnings later.

@mathbunnyru mathbunnyru merged commit 425794a into jupyter:master Nov 10, 2021
@mathbunnyru
Copy link
Member

Done

@maresb maresb deleted the quick-fix-uid branch November 10, 2021 12:32
@consideRatio
Copy link
Collaborator

❤️ 🎉 thanks for your work on making this land properly @maresb @mathbunnyru!!

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.

3 participants