Skip to content
This repository has been archived by the owner on Sep 7, 2018. It is now read-only.

Support for env variables ending with _FILE #166

Merged
merged 2 commits into from
Jun 1, 2018
Merged

Conversation

efrecon
Copy link
Contributor

@efrecon efrecon commented May 20, 2018

This slight modification to the main run script will recognise environment variables starting with GF_ and ending with _FILE. Whenever such variables exist, the content of the file that they point at will be read and used to set the same environment variable, but without the _FILE ending. This is handy when using Docker secrets and solves #149 . The use of the ending _FILE is inspired by other official images, such as postgres for example.

@efrecon
Copy link
Contributor Author

efrecon commented May 29, 2018

Any comment/news about this PR?

@xlson
Copy link
Contributor

xlson commented May 30, 2018

@efrecon I haven't had a chance to look at it properly yet but I'll take a look tomorrow. Thanks for reminding me.

@xlson xlson self-assigned this May 30, 2018
@xlson xlson self-requested a review May 30, 2018 14:31
@xlson
Copy link
Contributor

xlson commented May 31, 2018

@DanCech any thoughts regarding this PR? Seems like a good addition to me.

@DanCech
Copy link
Contributor

DanCech commented May 31, 2018

The concept is an interesting one, the code can be simplified to:

for VAR_NAME in $(env | grep '^GF_[^=]\+_FILE=.\+' | sed -r "s/([^=]*)_FILE=.*/\1/g"); do
  VAR_NAME_FILE="$VAR_NAME"_FILE
  if [ "${!VAR_NAME}" ]; then
    echo >&2 "ERROR: Both $VAR_NAME and $VAR_NAME_FILE are set (but are exclusive)"
    exit 1
  fi
  echo "Getting secret $VAR_NAME from ${!VAR_NAME_FILE}"
  export "$VAR_NAME"="$(< "${!VAR_NAME_FILE}")"
  unset "$VAR_NAME_FILE"
done

@efrecon
Copy link
Contributor Author

efrecon commented May 31, 2018

Fine by me, do you want to take in the PR and make the change manually afterwards?

@DanCech
Copy link
Contributor

DanCech commented May 31, 2018

I committed the tweaked version, @efrecon can you confirm that the updated code works for you?

@efrecon
Copy link
Contributor Author

efrecon commented Jun 1, 2018

Confirmed that your changes worked @DanCech , good to go @xlson .

@xlson
Copy link
Contributor

xlson commented Jun 1, 2018

Thanks guys! Merging this now!

@xlson xlson merged commit 89b7c50 into grafana:master Jun 1, 2018
@xlson xlson removed their request for review June 1, 2018 14:17
@efrecon
Copy link
Contributor Author

efrecon commented Jun 2, 2018

This ought to be documented somewhere. The README from this repo is automatically copied onto the Docker hub and basically points out the general documentation on the Grafana main site. A new sub-section under the docker installation documentation perhaps? Maybe I am blind, but I can't seem to find the section on Docker under the grafana.org project. Where is the source for the documentation? (sorry...)

@xlson
Copy link
Contributor

xlson commented Jun 4, 2018

@efrecon Definitely. There's an issue in the main repo grafana/grafana#12132 to track that need. If you feel up for it you're more than welcome to, otherwise I'll make sure it happens before the 5.2 release. This file needs to be updated: docker.md

All documentation pertaining to running Grafana as a Docker container sits in the install instruction you linked to. We keep the bare minimum in the README.md here and on Docker Hub.

@xlson
Copy link
Contributor

xlson commented Jun 4, 2018

@efrecon it looks like we'll be doing a beta release very soon so I'll write up the docs myself. Thanks anyway.

@efrecon
Copy link
Contributor Author

efrecon commented Jun 11, 2018

Feel free to pick anything (or nothing) from here if necessary.

@xlson
Copy link
Contributor

xlson commented Jun 12, 2018

@efrecon Thanks for sharing your post.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants