-
Notifications
You must be signed in to change notification settings - Fork 18
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
Support reading configuration from config files #71
Merged
romanwozniak
merged 14 commits into
caraml-dev:main
from
romanwozniak:config-refactoring
Nov 22, 2022
Merged
Support reading configuration from config files #71
romanwozniak
merged 14 commits into
caraml-dev:main
from
romanwozniak:config-refactoring
Nov 22, 2022
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 tasks
romanwozniak
requested review from
krithika369,
ariefrahmansyah and
tiopramayudi
November 21, 2022 10:29
krithika369
approved these changes
Nov 21, 2022
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.
Left some questions for clarification. LGTM. Thanks!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
As of now, all the configuration values are passed to gojek/mlp via environment variables. With the growing complexity of the application, the complexity of its configuration grows proportionally, so this way of passing config data to the application is no longer convenient. This is because it requires some sort of encoding to pass complex struct/object data (i.e. serializing data as JSON-string. Example: charts/mlp/values.yaml#L47) to the application.
This PR does the necessary refactoring to allow passing runtime configuration to the application via YAML config files, while still making it possible to override sensitive config keys via env variables:
Changes:
Internally, knadh/koanf is used to parse and deserialize config data. Despite some similarity with spf13/viper, knadh/koanf doesn't lowercase config keys and generally feels like a better-maintained alternative to viper.
Breaking Changes:
Streams
/Teams
configuration was refactored to represent theStream
>Team
hierarchy. PreviouslyStream
andTeam
were two independent slices of strings, so MLP project can be created with any stream/team combination, which breaks the one-to-many relationship between stream and team.Now, the
Team
configuration is completely removed from the top-level configuration and instead, the type ofStream
configuration is changed tomap[string][]string
to hold the mapping between streams and the teams belonging to that stream.Before:
After:
Applications' UI was updated to support this refactoring too.