-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add -quiet to nomad node status command. #12426
Conversation
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 is a great start. I am wondering if we should look at integrating this with the Meta struct so that we can pass this along to any status context. If you look at the generic status command, I think you can see how that would be useful.
@tgross Do you think that's a good idea?
I like this idea in theory but generally we've kept list-specific args (ex. pagination) in their own command structs so that we're not polluting the other commands with arguments that can only ever apply to lists. Ex. if we moved this into |
Yeah that wouldn't be a great UX. I'll write up an issue around list command args and add it to the backlog with a link here. |
command/node_status.go
Outdated
// Return nothing if no nodes found | ||
if len(nodes) == 0 { | ||
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.
Moving this check is causing this test to fail: node_status_test.go#L210-L213
. I think the right behavior is to move all the args validation above any query, so that we never make the client.Nodes().List()
API call if the user has passed args that will prevent us from showing the result anyways.
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.
Fixed the failing test, and added two new tests. I have also squashed the commits into 2 logical commits.
Signed-off-by: Shishir Mahajan <smahajan@roblox.com>
Signed-off-by: Shishir Mahajan <smahajan@roblox.com>
@tgross I have addressed your |
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!
Smoke-tested with my local cluster:
$ nomad node status
ID DC Name Class Drain Eligibility Status
286e8737 dc1 nomad-client1 vagrant false eligible ready
cbb2e80d dc1 nomad-client0 vagrant false eligible ready
27aa3c5f dc1 nomad-client2 vagrant false eligible ready
83488af7 proxy proxy vagrant false eligible ready
$ nomad node status -quiet
286e8737-f730-3189-594b-fc5d6b5efe66
cbb2e80d-1d00-f1fc-b599-8d7717a2147f
27aa3c5f-57fa-9809-cd43-ac308d2c31a5
83488af7-0a69-770d-3a33-bb2e20a4e057
$ nomad node status -verbose -t '{{range .}}{{.ID}}
{{end}}'
286e8737-f730-3189-594b-fc5d6b5efe66
cbb2e80d-1d00-f1fc-b599-8d7717a2147f
27aa3c5f-57fa-9809-cd43-ac308d2c31a5
83488af7-0a69-770d-3a33-bb2e20a4e057
$ nomad node status -verbose
ID DC Name Class Address Version Drain Eligibility Status
286e8737-f730-3189-594b-fc5d6b5efe66 dc1 nomad-client1 vagrant 192.168.56.3 1.2.6-dev false eligible ready
cbb2e80d-1d00-f1fc-b599-8d7717a2147f dc1 nomad-client0 vagrant 192.168.56.2 1.2.6-dev false eligible ready
27aa3c5f-57fa-9809-cd43-ac308d2c31a5 dc1 nomad-client2 vagrant 192.168.56.4 1.2.6-dev false eligible ready
83488af7-0a69-770d-3a33-bb2e20a4e057 proxy proxy vagrant 192.168.56.69 1.2.6-dev false eligible ready
$ nomad node status -quiet -json
-quiet cannot be used with -verbose or -json
$ nomad node status -quiet -verbose
-quiet cannot be used with -verbose or -json
$ nomad node status -json | jq '. | length'
4
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. |
More details
here
/cc @tgross @DerekStrickland
Signed-off-by: Shishir Mahajan smahajan@roblox.com