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

[WIP] thanos ruler compoment #534

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

[WIP] thanos ruler compoment #534

wants to merge 8 commits into from

Conversation

trouaux
Copy link

@trouaux trouaux commented Nov 26, 2024

closes #351

@trouaux trouaux force-pushed the thanos-ruler-component branch 2 times, most recently from 709212f to 5531122 Compare November 27, 2024 12:27
Copy link
Contributor

@viennaa viennaa left a 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.

retention: {{ $.Values.ruler.retention | quote }}
{{- end }}
{{- if $.Values.rbac.create }}
serviceAccountName: {{ $.Values.serviceAccount.name }}
Copy link
Contributor

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:

Suggested change
serviceAccountName: {{ $.Values.serviceAccount.name }}
serviceAccountName: {{ include "release.name" . }}-ruler

{{- if $.Values.ruler.retention }}
retention: {{ $.Values.ruler.retention | quote }}
{{- end }}
{{- if $.Values.rbac.create }}
Copy link
Contributor

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.


{{ if .Values.ruler.enabled }}
apiVersion: apps/v1
kind: Deployment
Copy link
Contributor

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.

@trouaux trouaux force-pushed the thanos-ruler-component branch from 5531122 to 6d0aa6f Compare November 27, 2024 14:51
@trouaux trouaux force-pushed the thanos-ruler-component branch from 72b3af0 to 46903d6 Compare November 29, 2024 10:21
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.

[thanos] Add Ruler component
2 participants