-
Notifications
You must be signed in to change notification settings - Fork 10
feat: add K8S+Helm recommended/standard labels #296
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
base: main
Are you sure you want to change the base?
Conversation
Example output :
|
Signed-off-by: Stephane Loeuillet <[email protected]>
e82811d
to
702a78a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this initiative @SLoeuillet !
I did keep the original labels like "app" but best practice is to "namespace" them like : canton.network/app
Happy for us to consider this as a follow-up issue / PR. I'll make a note on our internal tracker.
Example output :
Yeah Ideally we'd have a way to check the Helm template rendering automatically (to avoid surprising changes)... some day...
Actual review:
In addition to my comment in the code we should IMO extend this fix to much more (all?) of the Splice Helm charts. For consistency and because it seems like an easy drive-by win (with a nice helper definition as suggested in my comment).
LMK if you're interested in making such changes to your PR! Either way, we can't directly merge external PRs here yet but I can apply "approved" changes here to our internal repo.
@@ -5,6 +5,13 @@ apiVersion: apps/v1 | |||
kind: Deployment | |||
metadata: | |||
labels: | |||
app.kubernetes.io/component: participant |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we extract this whole block into a reusable definition in cluster/helm/splice-util-lib/templates/_helpers.tpl. This avoids code duplication (this is a fairly large block that we'll want in many places).
I'd also argue that this is more consistent to our goal state for managing these Helm templates. See for example include "splice-util-lib.service-account"
which we added recently.
I must admit that I didn't touch other components as I'm on Validator deployment only for now I open PRs once I find some things which would make multiple deployments on the same K8S cluster fail as my intermediate goal is to have a validator on devnet, 1 on testnet & 1 on mainnet on the same cluster, and more distant goal, same for SV Exemple : in previous PR, hardcoded PVC volume name, but it could be hardcoded DB name or DB username in case you use a unique PG server |
Add standard/recommended labels as per :
https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/
https://helm.sh/docs/chart_best_practices/labels/
I did keep the original labels like "app" but best practice is to "namespace" them like : canton.network/app
@isegall-da && @moritzkiefer-da, it supersedes : digital-asset/decentralized-canton-sync#365