-
Notifications
You must be signed in to change notification settings - Fork 145
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
Require a setting to be provided by the environment (passwords etc.) #251
Comments
+1 |
1 similar comment
+1 |
@jonaskello I agree that this would be a good feature as it would allow to follow the best practice you're mentioning. So to anyone interested, please propose a PR for this feature, it will be very welcome! |
Hey Marc! The logical way to implement this would be to assume that the evn var is required if the convict({
format: String,
env: 'IMPORTANT_PASSWORD'
}, { env: {}}) // throws But this would introduce a breaking change to the library, so the second best option is to introduce a convict({
format: String,
env: 'IMPORTANT_PASSWORD',
required: true
default: 'wow' // this is ignored and can be omitted
}, { env: {}}) // throws The What do you think? |
Can be fixed by #313 ? |
Technically yes, but for my needs it would be an overkill 💪 |
Why not a custom format which accept |
/**
* To require an env var
* use
* default: '',
* format: 'required-string',
*/
convict.addFormats({
'required-string': {
validate: (val: any): void => {
if (val === '') {
throw new Error('Required value cannot be empty')
}
},
coerce: (val: any): any => {
if (val === null) {
return undefined
}
return val
}
}
}) If anyone else is looking for a solution |
I'm thinking the case of a 12factor app where for example the database password is in the environment. We don't want to commit the production password to the config schema. Currently there seems to be no good way to declare that a setting is required to always be set from the enviroment although this is AFAIK best practice. In development you set it from a .env file that is not committed. This is related to #29 but seeing how that is closed I think a new issue could be useful.
The text was updated successfully, but these errors were encountered: