[FIXED] Override from command line not always working #578
Conversation
There were some cases where override would not work. Any command line parameter that would be set to the type default value (false for boolean, "" for string, etc) would not be taken into account. I moved all the flags parsing and options configuration into a new function, which may help reduce code duplication in NATS Streaming. The other advantage of moving this in a function is that it can now be unit tested. I am also removing call to `RemoveSelfReference()` which attempted to remove a route to self, which has been already solved at runtime with detecting and ignoring a route to self. This function would be invoked only when routes were defined in the configuration file, not in the command line parameter. Removing this call also solves an user issue (#577) Resolves #574 Resolves #577
main.go
Outdated
| } else if showHelp { | ||
| // Create a FlagSet and sets the usage | ||
| fs := flag.NewFlagSet("nats-server", flag.ExitOnError) | ||
| fs.Usage = func() { | ||
| usage() | ||
| } |
There was a problem hiding this comment.
Very minor nitpick, but this could just be fs.Usage = usage
There was a problem hiding this comment.
No we can't. It fails with Using usage() as a value otherwise.
There was a problem hiding this comment.
Odd, I don't know why that doesn't work as usage is a package-level function. https://play.golang.org/p/oDrPCpgOpj
There was a problem hiding this comment.
Feeling stupid... this is because I did fs.Usage = usage() instead of fs.Usage = usage. Will fix that. Thanks.
| } else if l > 2 { | ||
| return fmt.Errorf("invalid signal parameters: %v", commandAndPid[2:]) | ||
| } | ||
| if err := ProcessSignal(Command(commandAndPid[0]), pid); err != nil { |
There was a problem hiding this comment.
I think the only reason ProcessSignal was exported was so it could be called from main.go. We could unexport it since it's not intended to be part of the public API, unless we consider that a breaking change.
There was a problem hiding this comment.
I don't think anybody would have use that, but it was indeed an exported function that is present in a public release (1.0.0+). Let me think about it.
There was a problem hiding this comment.
Also, I was passing the flagSet to this function because at one point during dev I needed it, which I don't right now. So will fix that for sure.
|
After discussion, we decided to leave |
|
lgtm |
This is just for tests since from main.go, the FlagSet is set with ExitOnError so Parse() would call os.Exit(2). Regardless, I wanted to add error checking and a test for that. Related to #578
This is just for tests since from main.go, the FlagSet is set with ExitOnError so Parse() would call os.Exit(2). Regardless, I wanted to add error checking and a test for that. Related to #578
After upgrading vendor directory for gnatsd, we can now use natsd.ConfigureOptions() from PR nats-io/nats-server#578 We follow this model for NATS Streaming and chain the two configuration of options. This will solve any issue that we had with options override.
This will allow NATS Streaming to provide its own version of what should be printed when various flags are set. Related to #578
After upgrading vendor directory for gnatsd, we can now use natsd.ConfigureOptions() from PR nats-io/nats-server#578 We follow this model for NATS Streaming and chain the two configuration of options. This will solve any issue that we had with options override.
There were some cases where override would not work. Any command
line parameter that would be set to the type default value (false
for boolean, "" for string, etc) would not be taken into account.
I moved all the flags parsing and options configuration into
a new function, which may help reduce code duplication in
NATS Streaming.
The other advantage of moving this in a function is that it
can now be unit tested.
I am also removing call to
RemoveSelfReference()which attemptedto remove a route to self, which has been already solved at runtime
with detecting and ignoring a route to self.
This function would be invoked only when routes were defined in
the configuration file, not in the command line parameter.
Removing this call also solves an user issue (#577)
Resolves #574
Resolves #577