AGE-182: only write explicit changes to config file#36
Conversation
AGE-182 Tiger CLI: Make sure config updates don't accidentally save global flag/env vars
See Slack thread. Currently, if you run I think what's probably happening is that when it saves the default service ID to the config file, it's also saving any config values that were provided as env vars or flags, which is NOT what we want. I imagine the same is true for any other commands that save to the config file (e.g. I think the problem is that this call to Save() is saving the config with all global flags and env vars applied (because of the way |
cevian
left a comment
cevian
left a comment
There was a problem hiding this comment.
brilliant solution! bravo. One nit: I'd really like to see unit tests for this before approving. This is the kind of thing claude can generate tests for really well and may catch some edge cases.
murrayju
force-pushed
the
murrayju/config-mgmt
branch
from
ef05f14 to
9ee3ee1
Compare
nathanjcochran
left a comment
nathanjcochran
left a comment
There was a problem hiding this comment.
Overall approach looks great to me. Tested (very) briefly and it seems to do what I'd expect.
My one minor suggestion is that I'd prefer we didn't use reflection to set config values in updateField. See my comment below on that.
internal/tiger/config/config.go
Outdated
| // updateField uses reflection to update the field in the Config struct corresponding to the given key. | ||
| // This is used to keep the struct in sync with the viper state when setting values. | ||
| func (c *Config) updateField(key string, value any) error { | ||
| v := reflect.ValueOf(c).Elem() | ||
| t := v.Type() | ||
|
|
||
| // Find field by json tag | ||
| for i := range t.NumField() { | ||
| field := t.Field(i) | ||
| tag := field.Tag.Get("mapstructure") | ||
|
|
||
| if tag == key { | ||
| fieldValue := v.Field(i) | ||
| if fieldValue.CanSet() { | ||
| fieldValue.Set(reflect.ValueOf(value)) | ||
| viper.Set(key, value) | ||
| return nil | ||
| } | ||
| } | ||
| } | ||
| return fmt.Errorf("field not found: %s", key) | ||
| } |
There was a problem hiding this comment.
I don't love this reflection approach, tbh. Generally best to avoid reflection if possible imo ("Reflection is never clear" is one of the Go proverbs). I'd honestly prefer a big switch statement (even if it looks redundant) like before. Can the switch just be extracted from the Set call and used for this purpose too? I guess not, because Set takes a string value? But maybe things could be restructured somehow? 🤔
I could probably be convinced that reflection is fine here, if you have strong feelings. But it did stand out to me.
There was a problem hiding this comment.
In a vacuum, I might have argued for reflection for this function. But, thinking about it more, we already had a switch-case for Set that does some validation, and we might as well re-use that here.
3 participants
The config system had a problem where the effective config (after resolving CLI args, env vars, and defaults) would be written to the config file when any config was intended to be saved.
This caused several problems, most annoying of which being that temporarily passed CLI flags would get persisted (like
-o json).The solution is to never use the global
viperinstance to write the config file. Instead, a new instance is created, and only populated with the file content before being updated as needed. Then this can be written without carrying in unwanted values.