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.

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

Json config file must be overriden by env variable and by command-line argument #26

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

VonC
Copy link

@VonC VonC commented Oct 30, 2019

Fix #25

I have added a "value copy" in item, in order to be able to restore that copied value after the JSON unmarshalling (

opts/node_parse.go

Lines 145 to 155 in 8ed9e9c

//second round, unmarshal directly into the struct, overwrites envs and flags
if c := n.internalOpts.ConfigPath; c != "" {
b, err := ioutil.ReadFile(c)
if err == nil {
v := n.val.Addr().Interface() //*struct
err = json.Unmarshal(b, v)
if err != nil {
return fmt.Errorf("Invalid config file: %s", err)
}
}
}
)

Before, the JSON unmarshalled directly in item.val meant that any parameter/environment variable (previously set in item.val) was overridden.

Now, a "--myparam myvalue" correctly takes precedence over a static default "myparam" value stored in a JSON file.
Same for a MYPARAM environment variable.

All tests are OK.

@jpillora
Copy link
Owner

jpillora commented Nov 4, 2019

Thanks for the PR! Took a quick look, I think the true solution to this is a multistage flags parse.

  • parse flags into plain strings
  • has config flag? Unmarshal json onto struct
  • Unmarshal remaining plain string flags onto struct (existing logic)

This removes the need to store copies on item, and keeps the code simpler (1 pass instead of 2)

This would require a pkg/flag replacement but also enables multi shorts (ps -aux) and flags after args.

I’ll consider this if the above turns out to be too complex. Need to evaluate it.

@VonC
Copy link
Author

VonC commented Nov 4, 2019

@jpillora OK. I don't fully get the multistage approach, so I look forward to see your patch.

In the meantime, I'll keep my fork: that item copy solution was simpler for me to implement.

I didn't test multi short options though. Are they currently supported by opts?

@hans-d
Copy link

hans-d commented Aug 22, 2020

any news on this ?

@jpillora
Copy link
Owner

Unfortunately no. I'd like to write my own flag parser, so I can peek at the flags without storing the result on the struct.

@VonC VonC force-pushed the json-config-file branch from 84486ba to fc73dd1 Compare March 12, 2022 00:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Override config file option value with option parameter
3 participants