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: configuration source precedence #980

Merged
merged 18 commits into from
Sep 26, 2019
Merged

feat: configuration source precedence #980

merged 18 commits into from
Sep 26, 2019

Conversation

sh0rez
Copy link
Member

@sh0rez sh0rez commented Sep 6, 2019 •
edited
Loading

Implements a more sophisticated approach on config source precedence. Rules are
as follows:

  1. Flag defaults
  2. .yaml-file
  3. User-supplied flag values

Some hacking had to be done to separate defaults from actual flag values to not
overwrite the yaml contents. See the excessive use of yaml.(Un)marshal.
If a better approach is known, I am happy to take it.
Note: This is resolved now, removed that

Multiple tests have been added as well to make dead sure this package behaves as desired.

@sh0rez sh0rez added component/loki type/enhancement Something existing could be improved labels Sep 6, 2019
@sh0rez sh0rez self-assigned this Sep 6, 2019
@sh0rez sh0rez marked this pull request as ready for review September 10, 2019 19:54
@sh0rez sh0rez force-pushed the config branch 2 times, most recently from f905f1a to c88b984 Compare September 14, 2019 11:56
rfratto
rfratto requested changes Sep 16, 2019
View reviewed changes
Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

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

This is cool! My only overall gripe is how much we're relying on yaml un/marshaling to implement the defaults and flag overrides. I found that it made it harder for me to understand what was going on; is there no better way of doing it?

cmd/loki/main.go Show resolved Hide resolved
rfratto
rfratto previously approved these changes Sep 17, 2019
View reviewed changes
Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

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

LGTM!

@sh0rez sh0rez requested review from cyriltovena and removed request for cyriltovena September 18, 2019 15:26
cyriltovena
cyriltovena reviewed Sep 19, 2019
View reviewed changes
pkg/cfg/flag.go Outdated Show resolved Hide resolved
sh0rez
sh0rez commented Sep 23, 2019
View reviewed changes
pkg/cfg/flag.go Outdated Show resolved Hide resolved
@sh0rez sh0rez changed the title feat: configuration source precedence WIP feat: configuration source precedence Sep 23, 2019
@sh0rez
Copy link
Member Author

sh0rez commented Sep 23, 2019 •
edited
Loading

This is not in a good state right now, DO NOT MERGE

@sh0rez sh0rez dismissed rfratto’s stale review September 23, 2019 20:47

Broke it afterwards, re-review required anyways

@sh0rez
Copy link
Member Author

sh0rez commented Sep 26, 2019

Re-wrote and drastically simplified the flag and defaults handling of this package, there is no shared state or whatsoever anymore!

Added tests for precedence as well, to make sure it all works as expected.

Mind having a look @rfratto @cyriltovena @slim-bean?

@sh0rez sh0rez changed the title WIP feat: configuration source precedence feat: configuration source precedence Sep 26, 2019
rfratto
rfratto reviewed Sep 26, 2019
View reviewed changes
Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

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

This is great! Thanks for taking the time to make the implementation easier to understand.

When someone approves this, can you hold off on merging until #1062 gets merged? Joe is working hard to move us over to modules and he asked us to hold off on changing Gopkg.lock

cmd/loki/main.go Outdated Show resolved Hide resolved
cmd/promtail/main.go Outdated Show resolved Hide resolved
pkg/cfg/files.go Show resolved Hide resolved
@sh0rez
Copy link
Member Author

sh0rez commented Sep 26, 2019

When someone approves this, can you hold off on merging until #1062 gets merged? Joe is working hard to move us over to modules and he asked us to hold off on changing Gopkg.lock

No need to do that, not even modifying dependencies here, we can discard these autogenerated changes of Gopkg.lock.

@sh0rez sh0rez force-pushed the config branch 2 times, most recently from ca28e5e to b6219de Compare September 26, 2019 14:10
@sh0rez
chore(vendoring): ghodss/yaml, kelseyhightower/envconfig
57e3f1d
@sh0rez
feat: pkg/cfg
79cd766 
Adds configuration package for managing various configuration sources with
correct precedence
@sh0rez
feat(loki): pkg/cfg
de9555f
@sh0rez
feat(promtail): pkg/cfg
11167e5
@sh0rez
fix(cfg): remove env support
c4c4048 
Not immediately needed and easy to reimplement, removed for now
@sh0rez
feat(cfg): Parse() wrapper
e8e538b 
Adds a convenience wrapper `cfg.Parse()` which unmarshals from flag and yaml
@sh0rez
test(cfg): flag unit tests
d1640ea
@sh0rez
test(cfg): Parse(), FlagDefaults()
503938d 
Adds tests for Parse(), furthermore introduces FlagDefaults, a safer wrapper for
FlagDefaultsDangerous that fails when dst is not zero valued.
@sh0rez
fix(loki): validation globals
d4d3804
@sh0rez
fix(pkg/cfg): distinguish set values from defaults
79b2ccc 
Previously, the flag functionality relied on comparing archived defaults with
the merged data after flag.Parse(). This inevitably led to a bug, that this
package was able to decide whether a value is on default, or set by user to the
same as default. This meant the user value was always lost.

This now switches it to a more traditional usage of the flag package:

1. flag.***Var already sets the default value on the pointer
2. intermediate sources can make changes here
3. flag.Parse() ONLY sets new values
@sh0rez
Revert "chore(vendoring): ghodss/yaml, kelseyhightower/envconfig"
16e43ae 
This reverts commit 9b1b6d39ac00be9e992fd35f371c7c64b005719b.
@sh0rez
fix(cmd/promtail): remove debug code
9680533
@sh0rez
fix(cfg): unify flagsets
c3abcac
@sh0rez
test(cfg): adapt tests to recent changes
b9b28e2
@sh0rez
test(cfg): precedence tests
376de3e 
Adds tests to ensure precendence is working correctly
@sh0rez
refactor(cfg): tidy TestParse()
16774ed
@sh0rez
feat(cfg): ignore empty json config
0082dcb
@sh0rez
fix: use correct logger
4b11ee8
rfratto
rfratto approved these changes Sep 26, 2019
View reviewed changes
Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

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

LGTM!

@sh0rez sh0rez merged commit 0024540 into grafana:master Sep 26, 2019
@sh0rez sh0rez deleted the config branch September 26, 2019 14:26
This was referenced Sep 26, 2019
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rfratto rfratto rfratto approved these changes

@slim-bean slim-bean Awaiting requested review from slim-bean

@cyriltovena cyriltovena Awaiting requested review from cyriltovena

Assignees

@sh0rez sh0rez

Labels
component/loki type/enhancement Something existing could be improved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sh0rez @rfratto @cyriltovena