-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
HostVolumes: Include volume info in nomad node status #6011
Conversation
d5ce4d7
to
4888b4c
Compare
597df75
to
5b222d3
Compare
5935e7a
to
4a1d99c
Compare
5b222d3
to
e4ac438
Compare
4a1d99c
to
d2ee5fc
Compare
e4ac438
to
84be3ed
Compare
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.
Code lgtm - Had a minor code stylistic issues that aren't blockers. Would love some basic tests though for merging.
84be3ed
to
307fe2d
Compare
bd34a58
to
22b16de
Compare
307fe2d
to
ac26f25
Compare
@notnoop Generally cleaned up a couple of parts of this command to make it a bit easier to understand what's going on.
|
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.
overall looks good to me. Nitpick about error messaging, but would be really nice to have some tests for this namespace.
command/node_status.go
Outdated
func (c *NodeStatusCommand) outputAllocInfo(client *api.Client, node *api.Node) error { | ||
nodeAllocs, _, err := client.Nodes().Allocations(node.ID, nil) | ||
if err != nil { | ||
return err |
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.
I'd return the error prefix here instead of the caller side. Had the function been queryAllocInfo
, having callers add the prefix would be reasonable imo.
return err | |
return fmt.Errorf("failed to query node allocations: %s", err) |
} | ||
|
||
c.Ui.Output(c.Colorize().Color(formatKV(basic))) | ||
return 0 |
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.
Terminating early here is very nice; it's much more obvious what the short
flag displays here imo - thanks for doing this!
5487089
to
78e4459
Compare
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
NOTE: When #5923 is merged into
f-host-volumes
, this PR should be targeted againstf-host-volumes
Short Output
Verbose Output