-
Notifications
You must be signed in to change notification settings - Fork 3
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
[WIP] thanos ruler compoment #534
base: main
Are you sure you want to change the base?
Conversation
709212f
to
5531122
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 tackling your first issue and opening this PR.
We need to change the deployment to a statefulset. Will continue the PR after this change is in.
Please add testing for the respective statefulset to come up.
Please fix the linting test.
thanos/charts/templates/ruler.yaml
Outdated
retention: {{ $.Values.ruler.retention | quote }} | ||
{{- end }} | ||
{{- if $.Values.rbac.create }} | ||
serviceAccountName: {{ $.Values.serviceAccount.name }} |
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.
Would go with a default here:
serviceAccountName: {{ $.Values.serviceAccount.name }} | |
serviceAccountName: {{ include "release.name" . }}-ruler |
thanos/charts/templates/ruler.yaml
Outdated
{{- if $.Values.ruler.retention }} | ||
retention: {{ $.Values.ruler.retention | quote }} | ||
{{- end }} | ||
{{- if $.Values.rbac.create }} |
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'd say this can default to true, if it is not set.
thanos/charts/templates/ruler.yaml
Outdated
|
||
{{ if .Values.ruler.enabled }} | ||
apiVersion: apps/v1 | ||
kind: Deployment |
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.
We need to have this as a statefulset. When we talked about it, I said deployment but forgot that is usually should be deployed as a sfs.
5531122
to
6d0aa6f
Compare
72b3af0
to
46903d6
Compare
closes #351