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 #761 expand pvc_name outside of init #820

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Ph0tonic
Copy link

@Ph0tonic Ph0tonic commented Jan 16, 2024

Partial fix for #761 by moving the expansion of pvc_name and secret_name outside of __init__.

\cc @yuvipanda

Copy link

welcome bot commented Jan 16, 2024

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@manics
Copy link
Member

manics commented Jan 16, 2024

I've added the breaking label since this changes method signatures that may be used by subclasses.

@consideRatio consideRatio reopened this Jun 1, 2024
@@ -2662,6 +2660,8 @@ async def _make_create_resource_request(self, kind, manifest):

async def _start(self):
"""Start the user's pod"""
pvc_name = self._expand_user_properties(self.pvc_name_template)
Copy link
Member

Choose a reason for hiding this comment

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

I've been looking at this in #744, and pvc_name should actually be one of the things persisted in Spawner state, because the pvc name should probably be preserved to avoid data loss even if it. Resolving the template here would ensure the new template is used and the old pvc is orphaned. But that's intended when overrides are used as in #761.

Should we:

  1. not try to track existing pvcs (status quo, this PR), or
  2. try not to lose data (what I'm trying to do in add 'safe' slug scheme #744)

If we should try not to lose data, how do we distinguish between a changed pvc for a profile (should change) vs config (should not change if prior pvc already created)?

@minrk minrk mentioned this pull request Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants