Skip to content
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

Helm: Fix regression in chart name #2406

Merged
merged 1 commit into from
Jul 24, 2020

Conversation

steven-sheehy
Copy link
Contributor

What this PR does / why we need it:
Fixes a regression caused by the Logstash PR #1822. That PR populated the fullnameOverride for sub-charts, breaking existing deployments and external resources depending upon the release-prefixed service name. fullnameOverride is an optional field for users to override the name, it shouldn't be set by charts. Helm best practice is that workload names contain the release name so that you can deploy the same chart multiple times to the same namespace.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated

Signed-off-by: Steven Sheehy <steven.sheehy@hedera.com>
@codecov-commenter
Copy link

Codecov Report

Merging #2406 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2406      +/-   ##
==========================================
- Coverage   61.67%   61.65%   -0.02%     
==========================================
  Files         160      160              
  Lines       13577    13577              
==========================================
- Hits         8373     8371       -2     
- Misses       4581     4583       +2     
  Partials      623      623              
Impacted Files Coverage Δ
pkg/querier/queryrange/downstreamer.go 95.87% <0.00%> (-2.07%) ⬇️

@@ -41,7 +38,6 @@ filebeat:

logstash:
enabled: false
fullnameOverride: logstash-loki
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is this is required for the default chart to work with logstash.

Unfortunately there is no way to know the service name within the value file. This is used by both the filebeat.yml and the main output of logstash.

Do you have an alternative ?

Copy link
Contributor Author

@steven-sheehy steven-sheehy Jul 23, 2020

Choose a reason for hiding this comment

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

I don't think it makes sense to break existing deployments for a disabled by default feature. A better alternative is to enhance the upstream chart to support templating of the necessary values so you can specify templated values in the values.yaml. For example, here is the config from my values.yaml to configure Grafana to use Loki (which by the way is broken currently due to this issue):

  grafana:
    additionalDataSources:
      - name: Loki
        type: loki
        access: proxy
        url: http://{{ .Release.Name }}-loki:3100
        jsonData:
          maxLines: 500

See the templating PR I did for Grafana's ingress annotations to see how it's done. I don't use logstash or know which fields have to be templated to be able to help with this.

Or if that's not feasible then simply documenting how users can adjust the chart for use with Logstash should suffice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gonna need some time to figure this out. May be we can add those override in the docs via —set for now as examples for logstash.

Copy link
Contributor

Choose a reason for hiding this comment

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

I found an other bug with those templates, for example promtail in the umbrella chart will use the stack template but with value from the subchart and the template is wrong.

Subchart and templates is not a fun area.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright I've sent a PR to make this part of the doc instead.

cyriltovena added a commit to cyriltovena/loki that referenced this pull request Jul 24, 2020
As per discussed in grafana#2406.

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

@cyriltovena cyriltovena merged commit dd68651 into grafana:master Jul 24, 2020
@steven-sheehy steven-sheehy deleted the fix-helm-name branch July 24, 2020 15:54
@steven-sheehy
Copy link
Contributor Author

Thanks @cyriltovena!

owen-d pushed a commit that referenced this pull request Jul 29, 2020
As per discussed in #2406.

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
mraboosk pushed a commit to mraboosk/loki that referenced this pull request Oct 7, 2024
Signed-off-by: Steven Sheehy <steven.sheehy@hedera.com>
mraboosk pushed a commit to mraboosk/loki that referenced this pull request Oct 7, 2024
As per discussed in grafana#2406.

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants