Skip to content
This repository was archived by the owner on Jul 28, 2026. It is now read-only.

Config file environment expansion - #281

Merged
rfratto merged 3 commits into
grafana-cold-storage:masterfrom
unbasical:master
Dec 9, 2020
Merged

Config file environment expansion#281
rfratto merged 3 commits into
grafana-cold-storage:masterfrom
unbasical:master

Conversation

@dcseifert

@dcseifert dcseifert commented Dec 9, 2020

Copy link
Copy Markdown
Contributor

PR Description

It would be a great feature to expand the configuration file with environment variables like described in issue #280.

Which issue(s) this PR fixes

Fixes #280 Expand config file with environment variables

Notes to the Reviewer

PR Checklist

  • CHANGELOG updated
  • Documentation added
  • Tests updated

@CLAassistant

CLAassistant commented Dec 9, 2020

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@rfratto

rfratto commented Dec 9, 2020

Copy link
Copy Markdown
Contributor

Thanks for working on this!

Following up from our discussion in #280:

Would disabling this feature by default (to avoid unexpected behavior) and enabling it via a switch be a valid option here?

I'd prefer it to work this way. The Loki implementation of this (including the use of github.com/drone/envsubst) would be preferable. Can you make this behavior optional and turn it off by default? Using a flag to enable it should be more than enough.

One other thing: please update the CHANGELOG and give yourself credit for working on this :)

@dcseifert

Copy link
Copy Markdown
Contributor Author

Implementation is done and CHANGELOG is updated :)
I tried to stick to the Loki implementation as close as possible.

Looking forward to your review.

@rfratto rfratto left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One small nit but this looks great! Do you mind if I add a commit for documenting this in the configuration reference? You're also free to take a crack at it if you want.

Comment thread pkg/config/config.go Outdated
Co-authored-by: Robert Fratto <robertfratto@gmail.com>
@dcseifert

Copy link
Copy Markdown
Contributor Author

No, feel free to add the docs!
I just skipped this step for now because I don't want to put the snippet from Loki into a wrong spot.

@rfratto

rfratto commented Dec 9, 2020

Copy link
Copy Markdown
Contributor

Looks like the permission for me to add commits is disabled. I'll just merge this now and follow up with docs. Thanks again!

@rfratto
rfratto merged commit c7db27c into grafana-cold-storage:master Dec 9, 2020
@mattdurham mattdurham mentioned this pull request Sep 7, 2021
3 tasks
mattdurham pushed a commit that referenced this pull request Nov 11, 2021
* Added config environment expansion

* Implemented optional env-var config substitution using CLI flag

* Update pkg/config/config.go

Co-authored-by: Robert Fratto <robertfratto@gmail.com>

Co-authored-by: Robert Fratto <robertfratto@gmail.com>
@github-actions github-actions Bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Apr 22, 2024
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Apr 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Expand config file with environment variables

3 participants