commands, config: tally and report unknown fields in config#636
commands, config: tally and report unknown fields in config#636odeke-em wants to merge 1 commit intotendermint:developfrom orijtech:filter-out-unknown-config-fields
Conversation
|
Question: should we be worried about future/backwards compatibility with this? Ie if the config struct changes, an old/new file could fail with a new/old version of tendermint. Or maybe we make it a command to just Also wondering if this is something that might be useful to PR into Viper itself ? |
That's a great question. Thinking about it, in deed, that's gonna cause future compatibility problems. But as you've mentioned in your follow up suggestion to add the We had talked offline about the PR into Viper, and the |
For tendermint/tendermint#636 Updates tendermint/tendermint#628 Add global configuration flag `--verify` which is used currently, primarily by Tendermint. Since `cli/setup.go` has the definitions for Tendermint's global configuration flags, place this one there too.
For tendermint/tendermint#636 Updates tendermint/tendermint#628 Add global configuration flag `--verify` which is used currently, primarily by Tendermint. Since `cli/setup.go` has the definitions for Tendermint's global configuration flags, place this one there too.
|
Requires tendermint/tmlibs#47 to be merged in first. |
|
Let's not put this in the upstream cli. Can we do it as a global hidden option on the tendermint command here? Maybe |
| https://camo.githubusercontent.com/915b7be44ada53c290eb157634330494ebe3e30a/68747470733a2f2f676f646f632e6f72672f6769746875622e636f6d2f676f6c616e672f6764646f3f7374617475732e737667 | ||
| )](https://godoc.org/github.com/tendermint/tendermint) | ||
| [](http://forum.tendermint.com:3000/) | ||
| [](https://cosmos.rocket.chat/) |
There was a problem hiding this comment.
@zramsay can u port this change to your readme update PR?
cmd/tendermint/commands/root_test.go
Outdated
|
|
||
| func TestParseInvalidConfig(t *testing.T) { | ||
| assert := assert.New(t) | ||
| cases := [...]struct { |
There was a problem hiding this comment.
Thanks for the suggestion @melekes, however in this case, when I get a failed test I'd like to easily jump to the index that failed, hence the [...] and subsequent 2: {}
There was a problem hiding this comment.
I'll update this as agreed upon on in the meeting.
cmd/tendermint/commands/root.go
Outdated
| if !viper.GetBool(cli.VerifyFlag) { | ||
| return conf, nil | ||
| } | ||
| tagName := "mapstructure" |
There was a problem hiding this comment.
why create a var? and not inline the string
There was a problem hiding this comment.
It was just an oversight. Thanks @melekes, yap I can inline that.
cmd/tendermint/commands/util.go
Outdated
| // Foo int `json:"foo"` | ||
| // Bar string `mapstructure:"bar"` | ||
| // recursively flattening anonymous struct fields by calling indexTaggedStructFields | ||
| // on them. |
There was a problem hiding this comment.
so how the result will look like? {tx.size: true}?
| } | ||
| if len(splits) > 1 { | ||
| switch splits[1] { | ||
| case "omitempty": |
There was a problem hiding this comment.
what if both of them are present? omitempty,squash
There was a problem hiding this comment.
So far I haven't seen an inclusion of both in "mapstructure" as "squash" is the equivalent "omitempty" and the idea is that they are mutually exclusive.
There was a problem hiding this comment.
We don't use both anywhere but I don't see why they would be mutually exclusive?
squash brings a substruct up to the current level in json serialization. omitempty leaves it out all together. I could imagine using both.
| // Bar string `mapstructure:"bar"` | ||
| // recursively flattening anonymous struct fields by calling indexTaggedStructFields | ||
| // on them. | ||
| func indexTaggedStructFields(v interface{}, tagName string) map[string]interface{} { |
There was a problem hiding this comment.
have you tried to search for such func? maybe it exists in go or viper?
There was a problem hiding this comment.
Not in Go at least, nor in Viper from what I've checked in their godoc https://godoc.org/github.com/spf13/viper
cmd/tendermint/commands/util.go
Outdated
| func filterOutUnknownFields(want interface{}, gotFieldsMap map[string]interface{}, tagName string, excusedFlags map[string]map[string]bool) error { | ||
| wantFieldsMap := indexTaggedStructFields(want, tagName) | ||
| unpermittedFields := make(map[string][]string) | ||
| globalScope := "global" |
There was a problem hiding this comment.
maybe this should be a const?
| // A usecase is to report fields that were set in | ||
| // the config file for a command but are unexpected. | ||
| func filterOutUnknownFields(want interface{}, gotFieldsMap map[string]interface{}, tagName string, excusedFlags map[string]map[string]bool) error { | ||
| wantFieldsMap := indexTaggedStructFields(want, tagName) |
There was a problem hiding this comment.
don't you think it will be better to extract this call and pass wantFieldsMap to filterOutUnknownFields?
func filterOutUnknownFields(wantFieldsMap map[string]interface{}, gotFieldsMap map[string]interface{}, excusedFlags map[string]map[string]bool) error {
There was a problem hiding this comment.
I could, however we still need to pass in the tagName for the internal caller inside https://github.com/orijtech/tendermint/blob/11f3c811332779f438f45322b433b399711ca748/cmd/tendermint/commands/util.go#L93.
If that weren't the case, I agree, I'd actually be nicer to extract it out.
There was a problem hiding this comment.
Can't we simplify this by having indexTaggedStructFields grab all sub structs and put them under keys like p2p.laddr ? Then we don't need to call indexTaggedStructFields in filterOutUnknownFields and don't care about vars like specializedCommand. Then we can do what Anton suggested too.
|
The code looks a bit complicated to me. I've added a few comments. Maybe we should add more comments or split existing functions into smaller functions. |
Codecov Report
@@ Coverage Diff @@
## config #636 +/- ##
=========================================
Coverage ? 58.58%
=========================================
Files ? 94
Lines ? 9134
Branches ? 0
=========================================
Hits ? 5351
Misses ? 3321
Partials ? 462 |
|
lets move this to next non breaking release so we can do more review |
|
what are we doing with this? |
Updates #628 Requires tendermint/tmlibs#47 first With new flag `--check-config`, we can now tally fields from the config file mapped to Go struct by tag "mapstructure", with those parsed out by viper and report an error if we find flags/fields that aren't accepted in the config. Note: We use the `--check-config` flag to maintain backward/forward compatibility with different versions of the file, as raised by @ebuchman at #636 (comment) * Exhibit: Given config in: ```shell proxy_app = "tcp://127.0.0.1:46658" moniker = "anonymous" fast_sync = true db_backend = "leveldb" log_level = "state:info,*:error" [rpc] laddr = "tcp://0.0.0.0:46657" [p2p] laddr = "tcp://0.0.0.0:46656" seeds = "" trex = "hey" picked = "bonjour" ``` Running: ```shell $ tendermint --home ~/.tendermint node --check-config ERROR: We've detected these unknown flags in your config file p2p: picked trex ```
03fc4e5 to
2dd47ea
Compare
|
do we want to renew this PR now that #792 is merged? |
|
I think it's a lot of extra code for not a lot of functionality gain. And I realized that now with #792, the whole |
Updates #628
With new flag
--check-config, we can now tally fields fromthe config file mapped to Go struct by tag "mapstructure",
with those parsed out by viper and report an error if we
find flags/fields that aren't accepted in the config.
Note: We use the
--check-configflag to maintain backward/forwardcompatibility with different versions of the file, as raised
by @ebuchman at
#636 (comment)
Given config in:
Running: