-
Notifications
You must be signed in to change notification settings - Fork 220
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
fix: Fixes labels and annotations #441
base: main
Are you sure you want to change the base?
fix: Fixes labels and annotations #441
Conversation
Signed-off-by: Jared Schmidt <jared.schmidt@nice.com>
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.
Additional if you change keys inside values.yaml you need to also adapt the README.md
# annotations: | ||
# foo: bar |
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.
Don't comment-out the key.
# annotations: | ||
# foo: bar |
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.
Don't comment-out the key.
# additionalAnnotations: | ||
# foo: bar |
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.
Don't comment-out the key.
# additionalLabels: | ||
# foo: bar |
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.
Don't comment-out the key.
relabellings: [] | ||
relabelings: [] |
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.
This is kinda breaking. But if we accept this, this change needs to be reflected into the 3 servicemonitor.yaml files:
- templates/manager/servicemonitor.yaml
- templates/webhooks/servicemonitor.yaml
- templates/metrics-server/servicemonitor.yaml
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.
Agreed, we should not break this
relabellings: [] | ||
relabelings: [] |
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.
This is kinda breaking. But if we accept this, this change needs to be reflected into the 3 servicemonitor.yaml files:
- templates/manager/servicemonitor.yaml
- templates/webhooks/servicemonitor.yaml
- templates/metrics-server/servicemonitor.yaml
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.
Agreed, we should not break this
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.
LGTM, but I have the same comments such as @mkilchhofer. Please process them and also undo all changes in the values file wrt commenting things
Provide a description of what has been changed
Labels and annotations have been fixed throughout the templates so that they actually work. The "annotation" key in the metadata is included or excluded depending on if it has content. The "{}" in several of the annotation overrides in the values.yaml file was causing the "helm template" command to break when a value was defined in an external values.yaml file. This bug has also been fixed. Several whitespace gobbling issues have been fixed where labels and annotations were being appended to the previous line.
Checklist
Fixes #
#437