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.

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

Environment variable resolution in configs #1198

Merged
merged 2 commits into from Jun 18, 2020

Conversation

viktorsomogyi
Copy link
Contributor

@viktorsomogyi viktorsomogyi commented May 20, 2020

This commit enables parsing config values from environment variables.
This is particularly useful if passwords can't be hardcoded in the config
files but instead passed to Cruise Control via an environment variable.
For instance if there is a MY_PASSWORD=test123 then defining the following
config in cruisecontrol.properties will be resolved on startup to the
actual password.

Config file on the disk: config.name=${env:MY_ENV_VAR}
Resolved runtime config: config.name=test123

This fixes #1197

This commit enables parsing config values from environment variables.
This is particularly useful if passwords can't be hardcoded in the config
files but instead passed to Cruise Control via an environment variable.
For instance if there is a MY_PASSWORD=test123 then defining the following
config in cruisecontrol.properties will be resolved on startup to the
actual password.

Config file on the disk:              Resolved runtime config:
config.name=${env:MY_ENV_VAR}   ==>   config.name=test123
@viktorsomogyi
Copy link
Contributor Author

@efeg would you please review this?

Copy link
Collaborator

@efeg efeg left a comment

Choose a reason for hiding this comment

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

Thanks for the patch! Left a couple minor comments.

@@ -541,4 +547,20 @@ public static void sanityCheckNonExistingGoal(List<String> goals, Map<String, Go

return balancednessCostByGoal;
}

/**
* Reads the configuration file, parses and validates the configs.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we also mention the properties that enable configs to be provided out of the configuration file?
Is it intentional to override these hardcoded properties if they were also defined in the configuration file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1st: acked, will do
2nd: well, I treat them as defaults here so if people want to do something else than what CC does by default, then they should be free to. What can I improve here?

Change-Id: I1759b4c0f0a2dba59f59d1aa7838aef2d8205e6d
Copy link
Collaborator

@efeg efeg left a comment

Choose a reason for hiding this comment

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

Thanks for the updates -- LGTM.

@efeg efeg merged commit d6323f7 into linkedin:master Jun 18, 2020
efeg pushed a commit to efeg/cruise-control that referenced this pull request Jun 19, 2020
This commit enables parsing config values from environment variables.
This is particularly useful if passwords can't be hardcoded in the config
files but instead passed to Cruise Control via an environment variable.
For instance if there is a MY_PASSWORD=test123 then defining the following
config in cruisecontrol.properties will be resolved on startup to the
actual password.

Config file on the disk:              Resolved runtime config:
config.name=${env:MY_ENV_VAR}   ==>   config.name=test123
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resolve environment variables in config on startup
2 participants