[promtail] fix: rework of #1119 to preserve backwards compatability/features

[ts-mini]

A potential solution for the problem in #1214

[ducnm0711]

Hi any update on merging this?
This seems to be a downgrade rather than upgrade to me :

• With chart version 3.11.0

image:
  tag: 2.5.0
config:
  lokiAddress: https://loki/api/v1/push
  snippets:
    extraClientConfigs: |
      tenant_id: tenant-a

• For version 4.2.0, you need to parse full file: | + client {{- toYaml . | nindent 4 }}

config:
  lokiAddress: https://loki/api/v1/push
  snippets:
    extraClientConfigs:
      tenant_id: tenant-a

  file: |
    server:
      log_level: {{ .Values.config.logLevel }}
      http_listen_port: {{ .Values.config.serverPort }}
    clients:
      - url: {{ tpl .Values.config.lokiAddress . }}
      {{- with .Values.config.snippets.extraClientConfigs }}
      {{- toYaml . | nindent 4 }}
      {{- end }}
    positions:
      filename: /run/promtail/positions.yaml
    scrape_configs:
      {{- tpl .Values.config.snippets.scrapeConfigs . | nindent 2 }}
      {{- tpl .Values.config.snippets.extraScrapeConfigs . | nindent 2 }}

[eviln1]

Hi 👋
#1119 was correctly marked as a breaking change ( and bumped the major version ) because .Values.config.snippets.extraClientConfigs 's type was changed from string to list, and chart's users need to know about that and adjust their config if needed.

However, I think that 4.0.0 is broken in the sense where the template doesn't render a valid - or at least, expected - promtail.yaml config, when used as intended (ie. using a list of extraClientConfigs).

If this PR changes the type of .Values.config.snippets.extraClientConfigs to string back again, then I believe it should also be marked as a breaking change, and bumped up a major version.

[ts-mini]

Sorry everyone, was a bit selfish and published my version locally to unblock. I've merged in some changes (the major version bump) + an indent fix I never pushed.

Apologies that this got lost on my plate of things to do.

[trevorwhitney]

WDYT of removing lokiAddress and just exposing an array of clients? I think that value is a hold over from the assumption of there only being one client, and I regret the way I chose to add extra clients while assuming there was always 1 configured by lokiAddress

[trevorwhitney]

instead of extraClients, how would you feel above exposing an array of clients and get rid of lokiAddress?

[aceat64]

WDYT of removing lokiAddress and just exposing an array of clients?

I like this, the default in values.yaml could still have the Loki address clearly set, but also show an inline example of additional options. For example:

# -- You can put here any keys that will be directly added to the config file's 'client' block.
# A minimum of one client is required.
clients:
  # The Loki address to post logs to.
  - url: http://loki-gateway/loki/api/v1/push
#    external_labels:
#      label: value
#    basic_auth:
#      username: loki
#      password: loki
#  - url: http://other-client-service.local/

[ts-mini]

I made changes given the feedback, deployed to our dev environment and seems to be 👍. Also took a stab at adding some verbage into the changelog to better communicate the breaking change(s).

[trevorwhitney]

Ahh, sorry I just saw you made the same changes as in #1425, which I just merged. I'm going to close this unless there's something from #1425 we missed that you have included here?