Server flags categorization #1125
Conversation
|
|
| @@ -19,7 +19,7 @@ const ( | |||
|
|
|||
| type Config struct { | |||
| // Enabled switches on support for multi tenant query federation | |||
| Enabled bool `yaml:"enabled"` | |||
| Enabled bool `yaml:"enabled" category:"advanced"` | |||
There was a problem hiding this comment.
I don't think this should be advanced, because this is a core (stable) feature. I left the same comment in a previous PR that was proposing to flag it as advanced.
There was a problem hiding this comment.
I changed the spreadsheet so hopefully it doesn't happen again.
| "server.register-instrumentation": Advanced, | ||
| } | ||
|
|
||
| func GetOverride(fieldName string) (category Category, ok bool) { |
There was a problem hiding this comment.
Can we also include a check to verify that all options from overrides map were actually used? This is to avoid having obsolete flags here.
There was a problem hiding this comment.
I think that would be helpful and have been trying to think of the best way to do something like this. Maybe a usage_test.go? I'll try to see if I can make something work.
There was a problem hiding this comment.
I'm going to merge this PR, since having this classification is very important. I would be glad if you could follow up with your test is a separate PR. An idea to test it is to check if all flags referenced here are included in cmd/mimir/help-all.txt.tmpl (maybe you have a smarter idea, otherwise you could fallback to this).
There was a problem hiding this comment.
I'll be sure to do the test still in another PR, thanks.
There was a problem hiding this comment.
I opened #1164 to make sure I don't forget about this.
pracucci
changed the title
What this PR does:
Categorizes flags as advanced for server flags and the tenant federation flag.
The server flags were tricky because the struct fields couldn't be tagged directly.
server.Configis inweaveworks/common, which also contained structs fromprometheus/exporter-toolkit. Instead of replicating the structs, I introduced an override mechanism to contain flag classification issues and provide an easier pathway for anyone else dealing with this issue in the future.Which issue(s) this PR fixes:
N/A
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]