-
Notifications
You must be signed in to change notification settings - Fork 6
Add initial support for docker secrets #5
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your PR.
I left some minor review comments.
lib/resolve/secrets.js
Outdated
|
||
const fs = require('fs'), | ||
path = require('path'), | ||
SECRETS_DIR = '/run/secrets', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to make it more generic. Make a parameter out of this value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is not possible, docker always search for secrets in this path.
lib/resolve/secrets.js
Outdated
SECRETS_DIR = '/run/secrets', | ||
output = {}; | ||
|
||
if (fs.existsSync(SECRETS_DIR)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to wrap it in a function. This way it would be easier to test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working on it.
test/configs/config-a1.json
Outdated
"a1": "a1", | ||
"a2": "a1", | ||
"env-var": "%{CONFIG_LOADER_TEST_VAR_A}" | ||
"env-var": "%{CONFIG_LOADER_TEST_VAR_A}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is sth wrong with whitespaces
README.md
Outdated
|
||
/tmp/config.json: | ||
``` javascript | ||
{ docks1: "%{DOCKER_SECRET1}", docks2: "%{DOCKER_SECRET2|def}" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there should be probably '#' instead of '%' in this example ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!, will correct it.
README.md
Outdated
- **basedir** - (String/Array, default: []) base directory (or directories) used for searching configuration files. Mind that `basedir` has lower priority than a configuration directory, process basedir, and absolute paths. | ||
- **replaceEnv** - (Boolean/String, default: false, constraint: A string value must be different than `replaceLocal`) if specified enables environment variable replacement. Expected string value e.g. `%` that will be used to replace all occurrences of `%{...}` with environment variables. You can use default values like: %{a.b.c|some-default-value}. | ||
- **replaceLocal** - (Boolean/String, default: '@', constraint: A string value must be different than `replaceEnv`) if specified enables configuration variable replacement. Expected string value e.g. `@` that will be used to replace all occurrences of `@{...}` with configuration variables. You can use default values like: @{a.b.c|some-default-value}. | ||
- **replaceDockerSecret** - (Boolean/String, default: false, constraint: A string value must be different than `replaceLocal` and `replaceEnv`) if specified enables docker secret file replacement. Expected string value e.g. `#` that will be used to replace all occurrences of `#{...}` with docker secret file content. You can use default values like: #{DOCKER_SECRET|some-default-value}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A link to a doc explaining docker secrets would be handy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, next revision.
config = override(opts.override, config, process.env); | ||
config = replaceEnvVariables(config, opts.replaceEnv, opts); | ||
config = replaceLocalVariables(config, opts.replaceLocal, opts); | ||
config = replaceDockerSecrets(config, opts.replaceDockerSecret, opts); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test it
Initial work to add support for docker secrets.