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

Rework of start.sh script, take 2 #1512

Merged
merged 10 commits into from
Nov 10, 2021

Conversation

maresb
Copy link
Contributor

@maresb maresb commented Nov 7, 2021

Towards fixing #1270 I am convinced by @consideRatio's approach in #1052.

His #1052 grew quite large, and generated a lot of interesting discussion. It also introduced some very valuable enhancements to start.sh to improve comments, messages, and make it more robust.

I have rebased a subset of his commits which I consider less controvertial. In theory, they should not significantly change anything except for the messages, but several edge cases should now be handled better.

I hope that this can be tested and approved. Then I hope to

@maresb
Copy link
Contributor Author

maresb commented Nov 7, 2021

Remaining commits from #1270 to cherry-pick:

Discussion about the $PATH issue: 282eee2#r399787678

Hmm, I was hoping that the CI would run the Docker tests. Looks like I have to figure out how to set them up locally...

@maresb maresb force-pushed the rework-start-part-1 branch 3 times, most recently from bc5f5fb to c37d4d7 Compare November 7, 2021 21:01
@maresb
Copy link
Contributor Author

maresb commented Nov 7, 2021

For my reference: build base-notebook and tag it as base-notebook. Then run

TEST_IMAGE="base-notebook" pytest -m "not info" base-notebook/test

Docker tests are passing.

base-notebook/start.sh Outdated Show resolved Hide resolved
base-notebook/start.sh Outdated Show resolved Hide resolved
base-notebook/start.sh Outdated Show resolved Hide resolved
base-notebook/start.sh Outdated Show resolved Hide resolved
@mathbunnyru
Copy link
Member

Hmm, I was hoping that the CI would run the Docker tests. Looks like I have to figure out how to set them up locally...

@maresb could you please rebase on the latest master?
I have just fixed the issue I created a few days ago.

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 will take a look today or tomorrow, this is quite a big change 👍

base-notebook/test/test_container_options.py Outdated Show resolved Hide resolved
@maresb
Copy link
Contributor Author

maresb commented Nov 8, 2021

@mathbunnyru thanks for the CI fix! Seems to be running now after rebase.

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 think this PR is great, there are only minor cleanups, and my lack of knowledge of bash :)

Could you please take a look?

base-notebook/start.sh Outdated Show resolved Hide resolved
base-notebook/start.sh Outdated Show resolved Hide resolved
base-notebook/start.sh Outdated Show resolved Hide resolved
base-notebook/start.sh Show resolved Hide resolved
base-notebook/start.sh Outdated Show resolved Hide resolved
base-notebook/start.sh Show resolved Hide resolved
base-notebook/start.sh Outdated Show resolved Hide resolved
base-notebook/start.sh Outdated Show resolved Hide resolved
base-notebook/start.sh Outdated Show resolved Hide resolved
base-notebook/start.sh Outdated Show resolved Hide resolved
@mathbunnyru
Copy link
Member

@consideRatio as this was your PR originally, could you please also check that everything seems OK to you?

@mathbunnyru
Copy link
Member

@maresb @consideRatio great work 👍

@maresb also, please, let's not rebase anymore (because I want to be able to see incremental changes and it doesn't work well with rebases). All the previous rebases were perfectly fine because it was kind of WIP and I didn't review the changes back then.

@maresb
Copy link
Contributor Author

maresb commented Nov 9, 2021

@mathbunnyru, agreed on the rebasing. My brain is dizzy from all the merge conflicts I had to resolve.

I'd like to take another review pass myself. I haven't had the chance to reexamine the changes yet as a whole.

base-notebook/start.sh Outdated Show resolved Hide resolved
Copy link
Collaborator

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

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

LGTM, I just made a small consistency tweak

@consideRatio
Copy link
Collaborator

consideRatio commented Nov 9, 2021

@maresb ❤️ 🎉 thanks for following up so thoroughly with this and being patient with review comments!

@mathbunnyru since you have approved this already, I'll go for a merge at this point!


I also triple verified that the latest commit that updated an echo statement shouldn't cause something to crash, and skip waiting for tests to succeed as the commit before passed.

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.

Populate home dir if it is empty
5 participants