Rework of start.sh script, take 2 #1512

maresb · 2021-11-07T19:58:59Z

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

bring in the remaining commits from [WIP] Rework of start*.sh scripts #1052 as PRs
implement Prepend $CONDA_DIR/bin instead of appending #1270 based on an approach which reverts 4ef8a78, likely with a few additional subtle modifications (PR now open).
Try to close Populate home dir if it is empty #1478
Write tests

maresb · 2021-11-07T20:05:02Z

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 · 2021-11-07T21:01:33Z

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

base-notebook/test/test_container_options.py

base-notebook/start.sh

mathbunnyru · 2021-11-08T13:40:48Z

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.

mathbunnyru

I will take a look today or tomorrow, this is quite a big change 👍

base-notebook/test/test_container_options.py

maresb · 2021-11-08T15:35:56Z

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

mathbunnyru

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

mathbunnyru · 2021-11-09T11:31:59Z

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

mathbunnyru · 2021-11-09T11:36:01Z

@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.

Co-authored-by: Ayaz Salikhov <[email protected]>

maresb · 2021-11-09T18:29:44Z

@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

consideRatio

LGTM, I just made a small consistency tweak

Co-authored-by: Erik Sundell <[email protected]>

consideRatio · 2021-11-09T23:58:37Z

@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.

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