-
Notifications
You must be signed in to change notification settings - Fork 148
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
Optionally expose node locality (region, zone, host) to pod #134
base: master
Are you sure you want to change the base?
Optionally expose node locality (region, zone, host) to pod #134
Conversation
2dd218d
to
391d5b6
Compare
391d5b6
to
c27da88
Compare
Rebased |
c27da88
to
b9946d2
Compare
b9946d2
to
f6ca266
Compare
Rebased |
cockroachdb/templates/job.init.yaml
Outdated
@@ -16,9 +16,9 @@ metadata: | |||
{{- with .Values.labels }} | |||
{{- toYaml . | nindent 4 }} | |||
{{- end }} | |||
annotations: |
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 don't like this approach, helm hooks annotations should always be present or someone who wants to add new annotations will override helms one
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.
You can add something like extraJobAnnotations and list them after helm hooks
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.
Hi! The purpose of this was to make it possible to remove the annotations.
I've moved this discussion into a new PR: #206
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.
Got it. Thanks!
f6ca266
to
3c92778
Compare
Rebased PR |
Discussion: Has this been investigated before? I imagine that most people want these locality tags in multi-region setups. This PR adds them from well-known node labels. I'm happy to transfer ownership of my proof-of-concept
initContainer
that mounts the node labels in the statefulset.TLDR: This pullrequest automatically adds the CLI start flag values