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

Promtail Helm Chart: Add support for passing environment variables #1529

Merged
merged 1 commit into from
Feb 5, 2020

Conversation

tourea
Copy link
Contributor

@tourea tourea commented Jan 15, 2020

  • Make Promtail's Helm chart support passing extra env variables
  • bump charts versions

What this PR does / why we need it:
This PR enables developers to pass environment variables to the promtail containers via helm similarly to how it's done for loki

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated

@claassistantio
Copy link

claassistantio commented Jan 15, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@sandeepsukhani sandeepsukhani left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I see a problem in setting the env var, added a suggestion with a fix for it.
Some other suggestion regarding bumping up of version.

@cyriltovena I would appreciate your thoughts on this since I am fairly new to helm charts.

production/helm/promtail/Chart.yaml Outdated Show resolved Hide resolved
production/helm/promtail/templates/daemonset.yaml Outdated Show resolved Hide resolved
@tourea
Copy link
Contributor Author

tourea commented Jan 22, 2020

@cyriltovena please review

@cyriltovena
Copy link
Contributor

You have conflicts, also why do you need env vars in promtails ? I don't think we support any env vars.

@Demon-DK
Copy link

Demon-DK commented Feb 5, 2020

.... why do you need env vars in promtails ? I don't think we support any env vars.

Hi, @cyriltovena

But I guess env vars support are really required feature
Look at the example below. I'm feeling lack of this feature in such example.
For a while environments in this example haven't worked :(
image

* Make Promtail's Helm chart support passing extra env variables
* bump charts versions
@tourea
Copy link
Contributor Author

tourea commented Feb 5, 2020

@cyriltovena We also had to pass environment variables to promtail to make it work in our environments.
Rebased; please review

@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1529      +/-   ##
==========================================
- Coverage   61.84%   61.78%   -0.07%     
==========================================
  Files         109      109              
  Lines        8304     8304              
==========================================
- Hits         5136     5131       -5     
- Misses       2774     2777       +3     
- Partials      394      396       +2
Impacted Files Coverage Δ
pkg/promtail/targets/tailer.go 73.56% <0%> (-2.3%) ⬇️
pkg/promtail/targets/filetarget.go 68.71% <0%> (-1.85%) ⬇️

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

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.

None yet

6 participants