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.

Sign up for GitHub

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

Jump to bottom

feat: Allow environment variables to override config values #1332

Merged
merged 1 commit into from
Aug 17, 2020
Merged

feat: Allow environment variables to override config values #1332

merged 1 commit into from
Aug 17, 2020

Conversation

mladkau
Copy link
Contributor

@mladkau mladkau commented Aug 14, 2020 •
edited
Loading

  • Ready for review
  • Follows CONTRIBUTING rules
  • Reviewed by Snyk internal team

What does this PR do?

Adding a wrapper around configstore to allow environment variables to override config values.

How should this be manually tested?

You can run the following to disable suggestions:

$ SNYK_CFG_DISABLESUGGESTIONS=true snyk test

Any background context you want to provide?

What are the relevant tickets?

https://snyksec.atlassian.net/browse/DC-881

Screenshots

Additional questions

@mladkau mladkau requested a review from a team August 14, 2020 08:22
@mladkau mladkau requested a review from a team as a code owner August 14, 2020 08:22
@ghost ghost requested review from aviadhahami and ekbsnyk August 14, 2020 08:22
mladkau
mladkau commented Aug 14, 2020
View reviewed changes
src/lib/user-config.ts
export const config = new Configstore(pkg.name);

class ConfigStoreWithEnvironmentVariables extends Configstore {
constructor(id, defaults = undefined, options = {}) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copied from configstore.

mladkau
mladkau commented Aug 14, 2020
View reviewed changes
src/lib/user-config.ts Outdated
}

public get(key: string) {
const env_key = `SNYK_${key.replace('-', '_').toUpperCase()}`
Copy link
Contributor Author

@mladkau mladkau Aug 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

We need to do a bit of string replace here so we can support things like disable-analytics which becomes SNYK_CFG_DISABLE_ANALYTICS.

@mladkau mladkau force-pushed the feat/config-env-value-override branch 3 times, most recently from 219b6f5 to 2f03ffc Compare August 14, 2020 08:38
@garethr
Copy link
Contributor

garethr commented Aug 14, 2020 •
edited
Loading

Is it SNYK_DISABLE_ANALYTICS or SNYK_CFG_DISABLEANALYTICS? One is used in the description and one in the comment?

From an aesthetic point of view, the former of these is much nicer.

@mladkau
Copy link
Contributor Author

mladkau commented Aug 14, 2020 •
edited
Loading

The comment was outdated. It is SNYK_CFG_DISABLE_ANALYTICS for the config option disable-analytics. However, it is SNYK_CFG_DISABLESUGGESTIONS for the config option disableSuggestions :-) Our config values are a bit inconsistent.

The comment points out that I had to substitute hyphens in config values with underscores for equivalent environment variables because hyphens are not allowed in environment variables.

@mladkau mladkau force-pushed the feat/config-env-value-override branch from 2f03ffc to 2662595 Compare August 14, 2020 08:58
JackuB
JackuB reviewed Aug 14, 2020
View reviewed changes
src/lib/user-config.ts Outdated

public get(key: string) {
const envKey = `SNYK_CFG_${key.replace(/-/g, '_').toUpperCase()}`;
return process.env[envKey] || super.get(key);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we worried about types here?
Afaik, configstore can return types like bool or number as well, but process.env will always be a string?

Copy link
Contributor Author

@mladkau mladkau Aug 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

For disableSuggestions I can see the following code:

const disableSuggestions = userConfig.get('disableSuggestions');
if (disableSuggestions) {
  config.disableSuggestions = disableSuggestions;
}

...

  let dockerSuggestion = '';
  if (options.docker && config.disableSuggestions !== 'true') {

Which suggests we look for a string here but in other cases we might not. We could try to eval the environment value but this will have security implications. We could also try to do a parseInt, parseBool here and try to parse integer and boolean values (everything which cannot be parsed would be a string). WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or the other way around, cast everything from configstore as String to be consistent? Currently it's not typed at all and on many places we are already doing the conversion in code…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've checked the places in the code which I could find and converting everything into a string or undefined should work ... (famous last words)

@mladkau mladkau force-pushed the feat/config-env-value-override branch from 2662595 to 073d679 Compare August 14, 2020 13:38
@mladkau
Copy link
Contributor Author

mladkau commented Aug 14, 2020

I think Boost having a firedrill might make the tests fail in the moment...

@mladkau mladkau force-pushed the feat/config-env-value-override branch 3 times, most recently from 44042b7 to 905878f Compare August 14, 2020 14:01
mladkau
mladkau commented Aug 14, 2020
View reviewed changes
src/lib/config.ts Outdated
config.timeout = +timeout ? +timeout : DEFAULT_TIMEOUT;
let timeoutString = userConfig.get('timeout');
if (timeoutString) {
let timeout = parseInt(timeoutString);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wanted to make the string conversion here a bit more explicit - can remove if anyone disagrees ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(actually nevermind)

@mladkau mladkau force-pushed the feat/config-env-value-override branch 2 times, most recently from 2409531 to c10fae2 Compare August 14, 2020 16:36
@mladkau mladkau force-pushed the feat/config-env-value-override branch from c10fae2 to 16fc432 Compare August 14, 2020 16:42
@github-actions
Copy link
Contributor

github-actions bot commented Aug 14, 2020 •
edited by mladkau
Loading

Expected release notes (by @mladkau)

features:
Allow environment variables to override config values (16fc432)

  • I hereby acknowledge these release notes are 🥙 AWESOME 🥙

mladkau
mladkau commented Aug 14, 2020
View reviewed changes
src/lib/config.ts
@@ -52,7 +52,7 @@ if (!config.org && org) {
// invalid (non-numeric) value will fallback to the default
const timeout = userConfig.get('timeout');
if (!config.timeout) {
config.timeout = +timeout ? +timeout : DEFAULT_TIMEOUT;
config.timeout = timeout && +timeout ? +timeout : DEFAULT_TIMEOUT;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opted in the end for this as it doesn't require too much code change and seems to be ok with the now explicit typing.

@JackuB JackuB requested a review from pavel-snyk August 17, 2020 11:02
JackuB
JackuB approved these changes Aug 17, 2020
View reviewed changes
Copy link
Contributor

@JackuB JackuB left a comment

Choose a reason for hiding this comment

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

We'll need helper methods like ensure[Number|Boolean] but this is already an improvement as it brings solid typing for user-config

@mladkau mladkau merged commit 90acae1 into master Aug 17, 2020
@mladkau mladkau deleted the feat/config-env-value-override branch August 17, 2020 12:34
@snyksec
Copy link

snyksec commented Aug 17, 2020

🎉 This PR is included in version 1.376.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@snyk-bot snyk-bot mentioned this pull request Jan 6, 2023
Open
@snyk-bot snyk-bot mentioned this pull request Apr 29, 2023
Open
@MaxMood96 MaxMood96 mentioned this pull request Jun 21, 2023
Open
@aliscco aliscco mentioned this pull request Jun 22, 2023
Open
@JosanaDH JosanaDH mentioned this pull request May 21, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JackuB JackuB JackuB approved these changes

@aviadhahami aviadhahami Awaiting requested review from aviadhahami

@ekbsnyk ekbsnyk Awaiting requested review from ekbsnyk

@pavel-snyk pavel-snyk Awaiting requested review from pavel-snyk

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mladkau @garethr @snyksec @JackuB