-
Notifications
You must be signed in to change notification settings - Fork 183
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
Generate labels and names using helm templates #685
Conversation
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 nice and clean, LGTM
selector: | ||
matchLabels: | ||
app: collection-sumologic-fluentd-logs | ||
sumologic/app: fluentd-logs | ||
- name: collection-sumologic-fluentd-metrics |
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.
Shoudl we leverage these on the ServiceMonitor name and labels (not the selector labels but the labels for the ServiceMonitor as well?
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.
What is the purpose of those labels? Do we use them anywhere in the sumologic?
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.
It’s more consistency. Should we be consistent about the label usage across all things we install.
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.
That makes sense, I read additionalLabels as "labels which will be added as dimension to the metric"
@@ -1,9 +1,9 @@ | |||
apiVersion: v1 | |||
kind: ConfigMap | |||
metadata: | |||
name: {{ template "sumologic.fullname" . }}-fluentd-logs | |||
name: {{ template "sumologic.metadata.name.logs" . }} |
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 think it will be more meaningfull if we use configmap
at the end
so the full name will be:
sumologic.metadata.name.logs.configmap
Given that Helm is stringly typed
it will be easier to reason about what is being used where.
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.
+1
402fa61
to
15a081e
Compare
465f4a0
to
89700bc
Compare
681eb44
to
8997fe5
Compare
1c37961
to
03a300c
Compare
annotations: | ||
{{ toYaml .Values.sumologic.setup.serviceAccount.annotations | indent 4 }} | ||
labels: | ||
app: {{ template "sumologic.labels.app" . }} | ||
app: {{ template "sumologic.labels.app.roles.serviceaccount" . }} |
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.
sumologic.labels.app.setup.roles.serviceaccount
annotations: | ||
{{ toYaml .Values.sumologic.setup.clusterRole.annotations | indent 4 }} | ||
labels: | ||
app: {{ template "sumologic.labels.app" . }} | ||
app: {{ template "sumologic.labels.app.roles.clusterrole" . }} |
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.
sumologic.labels.app.setup.roles.clusterrole
?
annotations: | ||
{{ toYaml .Values.sumologic.setup.clusterRoleBinding.annotations | indent 4 }} | ||
labels: | ||
app: {{ template "sumologic.labels.app" . }} | ||
app: {{ template "sumologic.labels.app.roles.clusterrolebinding" . }} |
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.
sumologic.labels.app.setup.roles.clusterrolebinding
?
{{- include "sumologic.labels.common" . | nindent 8 }} | ||
spec: | ||
serviceAccountName: {{ template "sumologic.fullname" . }} | ||
serviceAccountName: {{ template "sumologic.metadata.name.roles.serviceaccount" . }} |
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.
👍
{{- template "sumologic.labels.app.setup" . }} | ||
{{- end -}} | ||
|
||
{{- define "sumologic.labels.app.setup.rolesserviceaccount" -}} |
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.
sumologic.labels.app.setup.roles.serviceaccount
?
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.
Some fixes are needed but overall IMO it's great and this will make refactoring a lot easier.
d33503a
to
a45a104
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.
LGTM 👍
Description
The purpose is to not lost track of relations between kubernetes objects
Testing performed