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

Add recommended chart labels alongside old labels (app.kubernetes.io/..., helm.sh/chart) #3404

Merged
merged 4 commits into from
May 8, 2024

Conversation

consideRatio
Copy link
Member

@consideRatio consideRatio commented Apr 21, 2024

This enables us to make a breaking change in z2jh 5 where user servers restarted once or more during z2jh 4 doesn't have to be restarted as part of the breaking change then. If we did it right away, network policy enforcement would fail for old servers for example.

Before z2jh 4, we should also get kubespawner to specify app.kubernetes.io/component alongisde component. This is done in jupyterhub/kubespawner#835.

Also, its nice to be able to provide time for people to adjust to the new labels with a period of overlapping labels. For example, grafana dashboards working with prometheus metrics could keep working in this period and at any time transition to refering to new labels, and then when we get z2jh 5 released such dashboards could have already adjusted.


This enables us to make a breaking change in z2jh 5 where user servers
restarted once or more during z2jh 4 doesn't have to be restarted as
part of the breaking change then. If we did it right away, network
policy enforcement would fail for old servers for example.

Before z2jh 4, we should also get kubespawner to specify
`app.kubernetes.io/component` alongisde `component`.

Also, its nice to be able to provide time for people to adjust to the
new labels with a period of overlapping labels. For example, grafana
dashboards working with prometheus metrics could keep working in this
period and at any time transition to refering to new labels, and then
when we get z2jh 5 released such dashboards could have already adjusted.
@consideRatio consideRatio force-pushed the pr/modern-label-naming branch from 0b44da9 to 06114e0 Compare April 24, 2024 10:15
@consideRatio consideRatio requested a review from manics April 24, 2024 11:18
@consideRatio consideRatio changed the title Add recommended chart labels alongside old labels Add recommended chart labels alongside old labels (app.kubernetes.io/..., helm.sh/chart) Apr 24, 2024
@manics
Copy link
Member

manics commented Apr 24, 2024

I think I understand what your intentions are.

test_spawn_basic already inspects some properties of the pod, do you think you could also verify the full set of labels are present? This will also make the impact of the future removal clearer.

@consideRatio consideRatio force-pushed the pr/modern-label-naming branch 3 times, most recently from d73782f to 5386112 Compare April 27, 2024 20:37
@consideRatio consideRatio force-pushed the pr/modern-label-naming branch from 944e3a7 to 2f8d0f8 Compare April 27, 2024 20:50
@consideRatio
Copy link
Member Author

Done @manics - I also fixed unrelated test failures via jupyterhub/action-k3s-helm#116,

Copy link
Member

@manics manics left a comment

Choose a reason for hiding this comment

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

Thanks for the updates!

jupyterhub/templates/_helpers.tpl Outdated Show resolved Hide resolved
jupyterhub/files/hub/jupyterhub_config.py Outdated Show resolved Hide resolved
@manics manics merged commit e7fe884 into jupyterhub:main May 8, 2024
15 checks passed
consideRatio pushed a commit to jupyterhub/helm-chart that referenced this pull request May 8, 2024
@consideRatio
Copy link
Member Author

Thank you for reviewing @manics!!! ❤️ 🎉 🌻

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.

2 participants