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

fix: do not fail on start when there are no persistent volume claims #1052

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

snglth
Copy link

@snglth snglth commented Apr 29, 2024

Close #1030

My solution seems dirty to me, so I'm very much open to suggestions on how to improve it.

)
# Create volumes if exists
volumes_manifest = tutor_env.pathjoin(context.root, "k8s/volumes.yml")
volumes_content = pathlib.Path(volumes_manifest).read_text()
Copy link
Contributor

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()?

Copy link
Author

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()?

Copy link
Contributor

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

"--selector",
"app.kubernetes.io/component=volume",
)
# Create volumes if exists
Copy link
Contributor

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.

Copy link
Author

Choose a reason for hiding this comment

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

I updated comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

K8s deployment fails when there's no persistent volume claims to apply
2 participants