-
Notifications
You must be signed in to change notification settings - Fork 21
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
[IA-5048] init script documentation #4778
Conversation
@@ -164,18 +253,6 @@ END | |||
docker restart $JUPYTER_SERVER_NAME | |||
docker restart $WELDER_SERVER_NAME | |||
|
|||
# This line is only for migration (1/26/2022). Say you have an existing runtime where jupyter container's PD is mapped at $HOME/notebooks, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should simplify and not support these old runtimes anymore
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4778 +/- ##
========================================
Coverage 74.78% 74.78%
========================================
Files 165 165
Lines 14948 14948
Branches 1178 1178
========================================
Hits 11179 11179
Misses 3769 3769
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
|
@@ -376,7 +400,9 @@ if [ ! -z "$JUPYTER_DOCKER_IMAGE" ] ; then | |||
mkdir -p ${WORK_DIRECTORY}/packages | |||
chmod a+rwx ${WORK_DIRECTORY}/packages | |||
|
|||
# TODO: update this if we upgrade python version | |||
# Install everything after having mounted the empty PD | |||
# This should not be needed anymore if the jupyter home is a directory of the PD mount point |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 this is a strong candidate for deletion, I can't imagine we still support runtimes where the home directory is not /home/jupyter
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I mean here is that if we do not mount the PD in the home.directory then we can install these in the container directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this! I got through gce-init.sh
, will pick up the rest later.
Made it, overall looks great. We should make sure to do some testing, especially around pause/resume which has more bash restructuring. Looks like some |
Thanks a bunch for the review, let me do a second pass and address your comments and I'll dig into the testing part next. |
@rtitle and @lucymcnatt I just tested creating, pausing, resuming, deleting with and without the PD for both a GCE and dataproc instance on my BEE and everything worked as expected 👌 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much cleaner now + clear comments, thanks for doing this work!
This reverts commit 10af4b4.
Jira ticket: https://broadworkbench.atlassian.net/browse/IA-5048
Summary of changes
What
Why
Testing these changes
What to test
Who tested and where
jenkins retest
orjenkins multi-test
.