-
Notifications
You must be signed in to change notification settings - Fork 416
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: do not fail on start when there are no persistent volume claims #1052
base: master
Are you sure you want to change the base?
Conversation
79af2da
to
8c798ed
Compare
) | ||
# Create volumes if exists | ||
volumes_manifest = tutor_env.pathjoin(context.root, "k8s/volumes.yml") | ||
volumes_content = pathlib.Path(volumes_manifest).read_text() |
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.
any particular reason for using Path here instead of open()?
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.
Just for brevity. Should I reimplement it using open()
?
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.
Tutor code is using open or io.open. It is better to keep the new code consistent with the existing pattern rather than having a new one. Thanks
tutor/commands/k8s.py
Outdated
"--selector", | ||
"app.kubernetes.io/component=volume", | ||
) | ||
# Create volumes if exists |
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.
nit: this comment can be updated to highlight when volumes are not created or vice versa.
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 updated comments
8c798ed
to
2e206f3
Compare
Close #1030
My solution seems dirty to me, so I'm very much open to suggestions on how to improve it.