-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[tempo-distributed] Adds global extraEnv support to deployed components #3333
Conversation
e0e60d6
to
32b098a
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.
Hey @rbrady could you please share the mimir reference for this change.
32b098a
to
8257470
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! Thanks! for the Contribution @rbrady, keep them coming!
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
Most of the deployed components did not apply the global extraEnv in the deployment templates. This resulted in having to duplicate extraEnv vars in the values/overrides files. This change uses the pattern from mimir to create an env: for a deployment if the .Values.global.extraEnv or .Values.<component>.extraEnv exists. It also adds the global extraEnv section to the values.yaml file. Signed-off-by: Ryan Brady <[email protected]>
8257470
to
2126cb6
Compare
@@ -70,9 +70,14 @@ spec: | |||
name: http-metrics | |||
- containerPort: {{ include "tempo.memberlistBindPort" . }} | |||
name: http-memberlist | |||
{{- with .Values.compactor.extraEnv }} |
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.
Is there any reason you changed .Values.compactor.extraEnv
to .Values.compactor.env
in the compactor but for distributor you left it the same?
This is just an example, other components are a bit inconsistent as well.
I think we should keep .Values.compactor.extraEnv
as is not a breaking change.
I realize is just a copy paaste issue because you used {{- with .Values.compactor.extraEnv }}
inside the if.
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.
@andreigorgan Thanks for pointing this out, I'll patch it by opening a PR
Most of the deployed components did not apply the global extraEnv in the deployment templates. This resulted in having to duplicate extraEnv vars in the values/overrides files. This change uses the pattern from mimir to create an env: for a deployment if the .Values.global.extraEnv or .Values..extraEnv exists. It also adds the global extraEnv section to the values.yaml file.
Closes #3332