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

Configuration: Support environment expansion in configuration #2837

Merged
merged 3 commits into from
Nov 5, 2020

Conversation

timbyr
Copy link
Contributor

@timbyr timbyr commented Oct 29, 2020

What this PR does / why we need it:
Inspired by and partially cloned from cortexproject/cortex#2147

You can use environment variable references in the config file to set values that need to be configurable during deployment.
The replacement is case-sensitive and occurs before the YAML file is parsed.
References to undefined variables are replaced by empty strings unless you specify a default value or custom error text.

To specify a default value, use:

${VAR:default_value}
Where default_value is the value to use if the environment variable is undefined.

Which issue(s) this PR fixes:
Fixes #2311

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated

Inspired by cortexproject/cortex#2147

Fixes grafana#2311

Adds -config.expand-env as flag to loki and promtail.
@CLAassistant
Copy link

CLAassistant commented Oct 29, 2020

CLA assistant check
All committers have signed the CLA.

@codecov-io
Copy link

Codecov Report

Merging #2837 into master will decrease coverage by 0.09%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2837      +/-   ##
==========================================
- Coverage   61.37%   61.28%   -0.10%     
==========================================
  Files         179      179              
  Lines       14438    14447       +9     
==========================================
- Hits         8862     8854       -8     
- Misses       4766     4783      +17     
  Partials      810      810              
Impacted Files Coverage Δ
pkg/cfg/files.go 22.72% <0.00%> (-5.85%) ⬇️
pkg/logcli/query/query.go 35.59% <0.00%> (ø)
pkg/promtail/positions/positions.go 46.80% <0.00%> (-11.71%) ⬇️
pkg/promtail/targets/file/filetarget.go 64.08% <0.00%> (-2.12%) ⬇️
pkg/canary/comparator/comparator.go 78.18% <0.00%> (+1.81%) ⬆️
pkg/querier/queryrange/downstreamer.go 97.64% <0.00%> (+2.35%) ⬆️

Copy link
Contributor

@achatterjee-grafana achatterjee-grafana left a comment

Choose a reason for hiding this comment

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

Doc content looks ok.

docs/sources/clients/promtail/configuration.md Outdated Show resolved Hide resolved
@slim-bean
Copy link
Collaborator

@timbyr super excited to see this, thank you! will try to get it reviewed in the next few says!

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.

LGTM with a minor nit.
Thanks for the PR!

pkg/cfg/files.go Outdated
@@ -36,13 +38,19 @@ func dJSON(y []byte) Source {
}

// YAML returns a Source that opens the supplied `.yaml` file and loads it.
func YAML(f string) Source {
func YAML(f string, envSubst bool) Source {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think either we should rename the variable to something like expandEnvVars and/or add a comment about what envSubst means. It would look clearer.

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.

Dam that was good.

@cyriltovena cyriltovena merged commit 5793c49 into grafana:master Nov 5, 2020
@cyriltovena cyriltovena added this to the 2.1 milestone Nov 5, 2020
@timbyr timbyr deleted the envexpansion branch November 7, 2020 22:28
@james-callahan
Copy link
Contributor

:( I just wasted 2 days trying to figure out this wasn't working for us

@oddlittlebird
Copy link
Contributor

Whoops. Sorry about that!

This is a lesson for all of us about being sure to label future-state docs.

@shairozan
Copy link

Same here. Team collectively spent over a day trying to figure out why this wasn't working. Only found this from an issue I had opened. Would we want to update the docs to at least avoid confusion until the code is included in a release?

@shairozan
Copy link

Or at least indicate in the docs that it's not on any current release and requires a separate flag?

@oddlittlebird
Copy link
Contributor

I have informed the team of what happened and reminded everyone to be vigilant for future-state docs. Sometimes, particularly with contributed content, we miss something.

I will file a PR to add a note to this one right now.

@james-callahan
Copy link
Contributor

@oddlittlebird the docs in that area are still missing any mention of -config.expand-env

@oddlittlebird
Copy link
Contributor

@oddlittlebird the docs in that area are still missing any mention of -config.expand-env

Please file an issue for this. Merged PRs are not the right place to discuss problems such as this.

@grafana grafana locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Environmental expansion
10 participants