-
Notifications
You must be signed in to change notification settings - Fork 0
Conversation
Coverage remained the same at 55.556% when pulling 8e33bb9cf83d01f4014fa10a400c1d3f799c0990 on port_by_env into 33570d8 on master. |
services/m3ctl/main/main.go
Outdated
|
||
if envPort != "" { | ||
if p, err := strconv.Atoi(envPort); err == nil { | ||
cfg.HTTP.Port = p |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: log a message indicating you're overriding the config value with an env var
services/m3ctl/main/main.go
Outdated
} else { | ||
logger.Fatalf("%s (%s) is not a valid port number", envPort, portEnvVar) | ||
} | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check should be outside the else, i.e.
if envPort != "" {
...
}
if cfg.HTTP.Port == 0 {
...
}
services/m3ctl/main/main.go
Outdated
@@ -47,7 +55,7 @@ type serverConfig struct { | |||
} | |||
|
|||
// NewHTTPServerOptions create a new set of http server options. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/New/new
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Things running in docker like to pass in ports to listen on. Adding this option.