CLOUDP-57855: List all available measurements for a given process, Atlas #85
Conversation
internal/description/description.go
Outdated
| @@ -53,6 +53,7 @@ A user’s roles apply to all the clusters in the project.` | |||
| ListDBUsers = "List Atlas database users for a project." | |||
| ListEvents = "List events for an organization or project" | |||
| UpdateDBUser = "Update a MongoDB dbuser in Atlas." | |||
| ProcessMeasurements = "Get process measurements." | |||
There was a problem hiding this comment.
I am happy to change these descriptions
There was a problem hiding this comment.
what about "Get measurements for a given host" ?
| return json.PrettyPrint(result) | ||
| } | ||
|
|
||
| func (opts *atlasMeasurementsProcessOpts) newHostInfo() (*hostInfo, error) { |
There was a problem hiding this comment.
[q] do you have more plans fro hostInfo?
I'd rather take advantage of go multi return and do something like
func (opts *atlasMeasurementsProcessOpts) getHostNameAndPort() (string, int, error)but up to you if you think that having the helper struct helps
if you want to keep the helper struct another thing you could do is define just
func newHostInfo(host string)(*hostInfo, error)and not link it to atlasMeasurementsProcessOpts this may let you reuse it in other contexts
| func (opts *atlasMeasurementsProcessOpts) newHostInfo() (*hostInfo, error) { | ||
| host := strings.SplitN(opts.host, ":", -1) | ||
| if len(host) != 2 { | ||
| return nil, errors.New("the host information must be in the format host:port") |
There was a problem hiding this comment.
small nit on usuability, let's return something like
fmt.Errorf("expected hostname:port, got %s", host)| return opts.init() | ||
| }, | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| opts.host = args[0] |
There was a problem hiding this comment.
[nit] depending on how you handle my above changes (function return string:int vs newHostInfo) you could change this to do that process here since you have no use for opts.host rather than to parse it later
with this I mean you could fo
opt.host, opt.port, err = opts.getHostAndPort(args[0])Or
opt.hostInfo, err = newHostInfo(args[0])
internal/description/description.go
Outdated
| @@ -53,6 +53,7 @@ A user’s roles apply to all the clusters in the project.` | |||
| ListDBUsers = "List Atlas database users for a project." | |||
| ListEvents = "List events for an organization or project" | |||
| UpdateDBUser = "Update a MongoDB dbuser in Atlas." | |||
| ProcessMeasurements = "Get process measurements." | |||
There was a problem hiding this comment.
what about "Get measurements for a given host" ?
|
|
||
| port, err := strconv.Atoi(host[1]) | ||
| if err != nil { | ||
| return "", 0, err |
There was a problem hiding this comment.
same as before, could we improve on the error and show something like, invalid port number, got %s
| globalOpts: newGlobalOpts(), | ||
| } | ||
| cmd := &cobra.Command{ | ||
| Use: "process", |
There was a problem hiding this comment.
We are missing the alias here
| return json.PrettyPrint(result) | ||
| } | ||
|
|
||
| func (opts *atlasMeasurementsProcessOpts) getHostNameAndPort(hostInfo string) (string, int, error) { |
There was a problem hiding this comment.
[nit] this could be just
| func (opts *atlasMeasurementsProcessOpts) getHostNameAndPort(hostInfo string) (string, int, error) { | |
| func getHostNameAndPort(hostInfo string) (string, int, error) { |
since you don't use opts in the body
There was a problem hiding this comment.
this also let's you reuse it later, we have a bunch of commands that may need this great method ;)
Proposed changes
Jira ticket: CLOUDP-57855
Checklist
make fmtand formatted my codeFurther comments
./bin/mongocli atlas measurements process cluster0-shard-00-00-ajlj3.mongodb-dev.net:27017 --granularity PT1M --projectId 5e4e593f70dfbf1010295836 -P personal --period PT1M