add workaround/option to select Sensu config file from within tool #17
Conversation
| @@ -61,9 +76,10 @@ func NewConfigFromFlagSet(flagset *configFlagSet) (*Config, error) { | |||
| if err := json.Unmarshal(buf, &cfg.config); err != nil { | |||
| return nil, err | |||
| } | |||
| return &cfg, nil | |||
There was a problem hiding this comment.
Would it be possible to replace the logic inside this if block with a call to NewConfigFromFile in order to avoid code duplication?
| } | ||
| return nil, errors.New("No config file is set") |
There was a problem hiding this comment.
I think we don't want to return an error here, because the client is able to subscribe to checks from the Sensu server via the SENSU_CLIENT_SUBSCRIPTIONS environment variable and you can use RABBITMQ_URI to specify the URI of the Sensu server, which I believe lets you run the client without a config file.
There was a problem hiding this comment.
Exactly, internally we only use the environment variables to configure the clients. It's a desired behavour.
|
I've cleaned up the duplicate code and also removed the error return to realign with your conventions. You've obviously thought about this more than me, but it seems to me using Environment variables like this deviates from the main sensu project conventions. More importantly though, from what I can tell you could hose the sensu server if the SENSU_CLIENT_NAME environment variable is fat-fingered or not set. In any case, thanks for the PR review |
| if err := json.Unmarshal(buf, &cfg.config); err != nil { | ||
| func NewConfigFromFlagSet(flagset *configFlagSet) (*Config, error) { | ||
| if flagset != nil && flagset.configFile != "" { | ||
| cfg, err := NewConfigFromFile(flagset, flagset.configFile) |
There was a problem hiding this comment.
you can just do: return NewConfigFromFile(flagset, flagset.configFile)
There was a problem hiding this comment.
You're right, thats much better
There was a problem hiding this comment.
@bparli i gave you some syntax feedbacks, you can use the golang linter and govet to catch these smells easily
| return cfg, nil | ||
| } else { | ||
| var cfg = Config{flagset, &configPayload{}} | ||
| return &cfg, nil |
There was a problem hiding this comment.
you don't need to assign a variable, you can directlydo return &Config{flagset, &configPayload{}}, nil
| return nil, err | ||
| } | ||
| return cfg, nil | ||
| } else { |
There was a problem hiding this comment.
this else is not really needed, you can just drop it
I am using your sensu-client-go package to extend one of our tools to also run as a standalone sensu check. Since the tool already has command line flags I wasn't able to set the Sensu configuration file from the command line as the NewConfigFromFlagSet function expects.