Skip to content

fix(helm): Do not strip whitespace in target definitions for querier and read deployments - #19464

Merged
trevorwhitney merged 3 commits into
grafana:mainfrom
jkroepke:helm/strip-read
Oct 9, 2025
Merged

fix(helm): Do not strip whitespace in target definitions for querier and read deployments#19464
trevorwhitney merged 3 commits into
grafana:mainfrom
jkroepke:helm/strip-read

Conversation

@jkroepke

@jkroepke jkroepke commented Oct 9, 2025

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

Follow-up of #19461 (Helm CI was red, was not good to merged)

Which issue(s) this PR fixes:

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • Title matches the required conventional commits format, see here
    • Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR
@jkroepke

jkroepke commented Oct 9, 2025

Copy link
Copy Markdown
Contributor Author

We should not strip whitespaces on the list of args, otherwise they will be in one line and the arg parser of loki gets wrong arguments,

level=error ts=2025-10-09T18:00:10.335274496Z caller=log.go:223 msg="error running loki" err="unrecognised module name: read- -legacy-read-mode=false"

https://github.com/grafana/loki/actions/runs/18384060163/job/52379407344#step:10:2271

@trevorwhitney trevorwhitney left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for following up with the CI fix, looks like the last one got merged inadvertently.

@jkroepke

jkroepke commented Oct 9, 2025

Copy link
Copy Markdown
Contributor Author

It seems like the Helm CI is not part of the required Checks, maybe this is confusing.

@trevorwhitney

Copy link
Copy Markdown
Collaborator

@jkroepke I don't know how to make a conditional check required, so no, the helm checks aren't required

@pull-request-size pull-request-size Bot added size/S and removed size/XS labels Oct 9, 2025
@jkroepke

jkroepke commented Oct 9, 2025

Copy link
Copy Markdown
Contributor Author

gateway was broken as well:

        location ^~ /ui {
          proxy_set_header X-Scope-OrgID $remote_user;
          proxy_pass       http://http.release-name-loki-querier.svc.default:3100%!(EXTRA string=cluster.local, string=3100)$request_uri;
        }
@trevorwhitney
trevorwhitney merged commit 7c309fb into grafana:main Oct 9, 2025
86 checks passed
@jkroepke
jkroepke deleted the helm/strip-read branch October 10, 2025 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2 participants