[WIP] add 'safe' slug scheme #744
Conversation
- always produces valid values - uses names, etc. as-is, when valid - uses stripped name--hash when needed
| return True | ||
|
|
||
|
|
||
| def is_valid_object_name(s): |
There was a problem hiding this comment.
This function ended up not being used because instead of multiple is_valid args, is_valid_default is used, which validates the subset of object names and labels (same as object name with max length of 63 instead of 255)
|
Hi, |
| rendered = template.format( | ||
| userid=self.user.id, | ||
| username=safe_username, | ||
| username=username, | ||
| escaped_username=safe_username, |
There was a problem hiding this comment.
Do we really need this extra parameter ?
| unescaped_servername=raw_servername, | ||
| escaped_servername=safe_servername, |
There was a problem hiding this comment.
Same, do we really need this extra parameter ?
| @@ -1809,18 +1836,32 @@ def _expand_user_properties(self, template): | |||
| safe_username = escapism.escape( | |||
There was a problem hiding this comment.
Shall we remove this legacy_escaped_username now, as it is not used anymore ?
| username = safe_username | ||
| servername = safe_servername |
There was a problem hiding this comment.
Maybe we could also move the code generating username and servername inside the if, we do not need to compute these steps if we are not using this non safe slug.
|
Hi, |
|
@minrk I looked at this in order to help nudge it towards merge, but I see that its a very breaking PR to enable this for an existing hub as for example if self.enable_user_namespaces:
self.namespace = self._expand_user_properties(self.user_namespace_template)
self.pod_name = self._expand_user_properties(self.pod_name_template)
self.dns_name = self.dns_name_template.format(namespace=self.namespace, name=self.pod_name)
self.secret_name = self._expand_user_properties(self.secret_name_template)
self.pvc_name = self._expand_user_properties(self.pvc_name_template)
if self.working_dir:
self.working_dir = self._expand_user_properties(self.working_dir)The changes to the labels are far less breaking as it won't affect kubespawner/z2jh/binderhub/grafana-dashboards etc - and only be breaking for external software considering the labels. I'm not landing in a concrete action point here, just summarizing this realization. |
|
@consideRatio yeah, I think that's exactly why I didn't finish this. I think the new scheme is fine and works better, but figuring out the transition is the hard part. |
closes #737
closes #498
implements scheme described here.
-2d)name--hashwhen neededKubeSpawner.slug_scheme = 'safe'Potential issues to address:
Opened early as a demo for discussion, not tested yet.