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

[kube-prometheus-stack] Add support for --enable-feature of alertmanager #4606

Merged
merged 2 commits into from
Jun 16, 2024

Conversation

Sheikh-Abubaker
Copy link
Contributor

@Sheikh-Abubaker Sheikh-Abubaker commented Jun 16, 2024

What this PR does / why we need it

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged)

Special notes for your reviewer

Checklist

  • DCO signed
  • Chart Version bumped
  • Title of the PR starts with chart name (e.g. [prometheus-couchdb-exporter])

Signed-off-by: Sheikh-Abubaker <sheikhabubaker761@gmail.com>
Comment on lines 50 to 55
{{- if .Values.alertmanager.enableFeatures }}
enableFeatures:
{{- range .Values.alertmanager.enableFeatures }}
- {{ . | quote }}
{{- end }}
{{- end }}
Copy link
Member

Choose a reason for hiding this comment

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

this should work, too?

Suggested change
{{- if .Values.alertmanager.enableFeatures }}
enableFeatures:
{{- range .Values.alertmanager.enableFeatures }}
- {{ . | quote }}
{{- end }}
{{- end }}
{{- with .Values.alertmanager.enableFeatures }}
enableFeatures:
{{- toYaml . | nindent 4 }}
{{- end }}

Copy link
Contributor Author

@Sheikh-Abubaker Sheikh-Abubaker Jun 16, 2024

Choose a reason for hiding this comment

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

sure it would, but if the features are listed without quotes, that is why I made use of quotes, but no worries if you want I can change it and in case of {{- with }} it completely slipped off my mind.

Copy link
Member

Choose a reason for hiding this comment

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

Could you explain, what you mean with the quotes? Do I miss something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I meant if there is something like this in values.yaml

alertmanager:
  enableFeatures:
    - feature1
    - "feature2"

using quote would ensure the template renders it correctly as string, like this:

enableFeatures:
  - "feature1"
  - "feature2"

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. Quotes are most of the time optional. They are only required, if strings are expected, but numbers are given.

with toYaml,

alertmanager:
  enableFeatures:
    - feature1
    - "feature2"

would result into:

alertmanager:
  enableFeatures:
    - feature1
    - feature2

Copy link
Contributor Author

@Sheikh-Abubaker Sheikh-Abubaker Jun 16, 2024

Choose a reason for hiding this comment

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

yes that's right it just slipped off my mind to add with toYaml, apart from this I have updated the code, checkout the latest commit.

Signed-off-by: Sheikh-Abubaker <sheikhabubaker761@gmail.com>
@jkroepke jkroepke merged commit 6f1bc9e into prometheus-community:main Jun 16, 2024
4 checks passed
@Sheikh-Abubaker Sheikh-Abubaker changed the title [kube-prometheus-stack] Add support for --enable-feature for alertmanager [kube-prometheus-stack] Add support for --enable-feature of alertmanager Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[kube-prometheus-stack] Add support for --enable-feature for alertmanager
2 participants