Skip to content
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 cluster and nodes #10

Merged
merged 3 commits into from
Nov 7, 2019
Merged

Add cluster and nodes #10

merged 3 commits into from
Nov 7, 2019

Conversation

SaaldjorMike
Copy link
Member

I've also added a separate commit to clean up a few things, including moving to a new uuid package.

This is the first time I'm touching this repo, so let me know if you need me to change anything. The main thing I needed was to extend the API with the new types/queries/mutations, and to a lesser extent expanding on the CLI tool itself. I'm sure the output of the CLI tool could be adjusted, but not 100% certain exactly what data to present to the user, especially since we don't yet support JSON formatted output.

cmd/nodes.go Outdated
func newNodesCmd() *cobra.Command {
cmd := &cobra.Command{
Use: "nodes",
Short: "Manage nodes",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you should make the distinction between nodes and cluster commands clear.
I was confused at first since in my mind "nodes" is part of cluster management.

The least you could do would maybe be include a bit more description in the "Short" field for both. Alternatively nest the nodes command under cluster

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I think I'll move nodes to a nested command under cluster.

Copy link
Contributor

@anagrius anagrius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done. I will leave it up to you what to do with the nodes vs cluster commands.

Copy link
Contributor

@jswoods jswoods left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

@SaaldjorMike SaaldjorMike merged commit ef08564 into master Nov 7, 2019
@SaaldjorMike SaaldjorMike deleted the mike/cluster_nodes branch November 7, 2019 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants