search

grafana / helm-charts

Apache License 2.0
1.67k stars 2.28k forks source link

[tempo-distributed] fixes extraEnv condition for compactor #3345

Closed msvechla closed 1 month ago

msvechla commented 1 month ago

Hello,

This fixes a bug introduced with https://github.com/grafana/helm-charts/commit/2126cb60a0974de3369ad210ebce8be786d4ccaa where the condition was incorrectly added as .Values.compactor.env, when the values are supplied via .Values.compactor.extraEnv

Sheikh-Abubaker commented 1 month ago

Could you please address the failed CI check, you need to bump the chart version to 1.18.3 in chart.yaml and readme.md to fix the CI lint

msvechla commented 1 month ago

@Sheikh-Abubaker thanks for the hint, done!

DreamingRaven commented 1 month ago

Also interested in seeing this get through. Is there anything blocking this? Can I help in any way? Seems its just reviewing atm.

For anyone who is waiting for this, you can simply set the env that is mistakenly used in the if condition manually to true so it can get through if you need to set the otherwise guarded extraEnv block:

  compactor:
    # TODO: remove once version is bumped to include pr
    # workaround for: https://github.com/grafana/helm-charts/pull/3345
    env: true # <-- workaround
    extraArgs:
    - "-config.expand-env=true"
    extraEnv:
    - name: S3_ACCESS_KEY
      valueFrom:
        secretKeyRef:
          name: minio
          key: root-user
          optional: false
    - name: S3_SECRET_KEY
      valueFrom:
        secretKeyRef:
          name: minio
          key: root-password
          optional: false

To get around the top if condition: https://github.com/grafana/helm-charts/blob/350783e92680eb98ff0c2b75eb0502a2dcc79fcb/charts/tempo-distributed/templates/compactor/deployment-compactor.yaml#L73-L81

Or set the global env instead.

Sheikh-Abubaker commented 1 month ago

@DreamingRaven the suggestion is great but the folks already using this might be confused due to this, so to keep the code consistent and simple reverting .Values.compactor.env to.Values.compactor.extraEnv seems reasonable.

DreamingRaven commented 1 month ago

Agreed. As marked it's only a temporary work around. This should not be the convention / standard. And it should not detract from the pr which fixes the actual issue.

Sheikh-Abubaker commented 1 month ago

@msvechla Thanks for the contribution! but I'd have to close this on behalf of #3364 which fixes the stated bug.