Add modern app.kubernetes.io labels alongside old

[consideRatio]

This is related to jupyterhub/zero-to-jupyterhub-k8s#3404, where I'd long term like to enable us to transition to using modern k8s labelling scheme, where labels outlined in jupyterhub/zero-to-jupyterhub-k8s#1867 are used instead of labels used in legacy helm charts.

This change is made to be entirely non-breaking.

[manics]

There's an ongoing discussion about how names (including pod labels) are normalised, and whether this can be changed in a backwards compatible manner:

• Server names starting with upper letter fail to start #737
• add 'safe' slug scheme #744
• fix: unneeded escaping of capital letters in k8s labels #791
• @minrk @yuvipanda

If we want to do the above this might be a good opportunity to set the new labels to whatever the new normalisation scheme is, and leave the old labels unchanged, avoiding some backward compatibility complexity. It won't avoid all complexity since it's not just labels that are affected.

[consideRatio]

If we want to do the above this might be a good opportunity to set the new labels to whatever the new normalisation scheme is

Are you refering to the opportunity of getting things out in a z2jh release including a new kubespawner version, or alongisde this PR? I see this PR as practically unrelated to what we do with other labels.

I think adding new labels instead of changing things in old could make sense - very worth considering.

[yuvipanda]

I'm pro using this as an opportunity to fix our myriad escaping issues! Some variant of #744 would be pretty amazing.

I don't currently have capacity to push on that though :(

[consideRatio]

I opened #836 to focus on the related discussion

[consideRatio]

After discussion in #836, do you think this is OK to review/merge @manics?

[sgaist]

Based on the discussion link and the current implementation, I would say it should be good to go in.

[consideRatio]

Thank you for reviewing @sgaist! - I'll go for a merge!

• I did a delayed self-review, and implementation wise this still seems OK to merge to me
• With Add recommended chart labels alongside old labels (app.kubernetes.io/..., helm.sh/chart) zero-to-jupyterhub-k8s#3404 merged, this is very relevant to also get merged as we otherwise just do a partial introduction of the modern app.kubernetes.io labels
• I takeaway I have from the related investigation and discussion is that this PR is decoupled from breaking changes on how {username} etc expands in other labels are resource names

[yuvipanda]

I very much commend you for working on this and merging it, @consideRatio. To redescribe a sentiment I described in jupyterhub/oauthenticator#735 (comment), it's really important to keep momentum of review going to enable contributors to feel motivated. Without enough contributor motivation, projects slowly wither and die. And I don't want that.

However, another part of that is being able to revert PRs as well, especially before things get released. With that, I think this PR should be reverted. I've my rationale below.

I'm really confused by this PR now, as it looks like in the state it was merged, it adds new labels but doesn't listen based on them. I also wish I had expressed myself more clearly in #835 (comment) - that's on me, I'm sorry. Let me try again:

1. We currently have a major issue with our escaping, that breaks silently under some circumstances based on what letter things start with - see Add modern app.kubernetes.io labels alongside old #835 (comment)
2. One part of the problem fixing this is that escaping and state management is hard to figure out correctly in a backwards compatible way.
3. However, we do have one advantage - we can change the key of the label as well, not just the escaping of the value.
4. So I'd have liked to see the modern applabels use the newer escaping (fix: unneeded escaping of capital letters in k8s labels #791, add 'safe' slug scheme #744) from the start, so we can make a clear determination of what escaping method is used where. Old labels use old escaping, new labels new escaping.
5. After that, we can introduce a traitlet like disable_old_labels (or similar) that will just completely stop the old style labels, and default that to False. And then a few releases later, default that to True.

So the fact that we did not have the new style labels was an advantage, and with this PR we lose that. It's a tool we should and could employ in #835 (comment), and I don't want to lose that.

So I'd say switching to new label styles must be blocked on fixing the issues in #835 (comment). That means either directing efforts towards those PRs, or waiting until someone else does. I don't think it's necessary to tie these into jupyterhub/zero-to-jupyterhub-k8s#3404, but if that is necessary, I think the path forward is for that to be reverted and for that to wait as well. I don't see what advantage simply repeating the label (without watching for it) gives us, and it makes us lose an important tool in being able to solve #737.

And finally, I understand if the moment has passed and the choice is made to just continue with it as is - because I didn't chime in before this was merged. There was enough time for that. So I'll accept a 'you should have spoken up earlier' response to this as well, and work to organize myself for making that happen better. I'm going to try to systematically spend x minutes every week reviewing and merging PRs now.

[yuvipanda]

Just helped another person with it on slack #737 (comment)

[minrk]

I think I can dedicate some time later this week or next to picking #744 back up with these new labels in mind and finally think through a transition path.

[yuvipanda]

yay thank you, @minrk

[yuvipanda]

In conversation with @minrk today, he corrected me that this PR doesn't actually add any values that need escaping. Sorry for not catching that correctly on my first round! This does mean I no longer think we need to revert this PR, although I'd still love for us to get #744 done along with this, but I don't think they are particularly connected anymore.

[yuvipanda]

And to quote @minrk from our conversation,

Yeah, I really think it’s a fine PR with very little impact

So I think it was ok to self-merge this too, @consideRatio. I was just wrong, and you were right in #835 (comment). I apologize for my mistake here.

I'm now going to try to spend a minimum of 15 minutes every day reviewing and merging PRs across the JupyterHub ecosystem, with a focus on encouraging more newer contributors. I think that's the systematic solution to the systemic problem I've encountered here. Specifically, I'll also try to read the code first rather than the comments, as I think that was the particular thing that tripped me up here.

Not the first time I've had an opinion that later turned out to be wrong, and not the last either. I appreciate everyone holding space for me to make mistakes, and I promise to do the same.