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

feat: group subcommands in command help #675

Merged
merged 9 commits into from
Feb 1, 2024
Merged

feat: group subcommands in command help #675

merged 9 commits into from
Feb 1, 2024

Conversation

phm07
Copy link
Contributor

@phm07 phm07 commented Jan 16, 2024

This PR adds subcommand groups, which make the help output more structured and easier to read. Conventions on which groups to add and how to name them is also added.

@phm07 phm07 self-assigned this Jan 16, 2024
Copy link

codecov bot commented Jan 16, 2024

Codecov Report

Attention: 161 lines in your changes are missing coverage. Please review.

Comparison is base (e8f3620) 45.32% compared to head (8cd52d5) 44.76%.

Files Patch % Lines
internal/cmd/server/server.go 0.00% 45 Missing ⚠️
internal/cmd/loadbalancer/load_balancer.go 0.00% 26 Missing ⚠️
internal/cmd/network/network.go 0.00% 17 Missing ⚠️
internal/cmd/floatingip/floatingip.go 0.00% 16 Missing ⚠️
internal/cmd/primaryip/primaryip.go 0.00% 15 Missing ⚠️
internal/cmd/volume/volume.go 0.00% 14 Missing ⚠️
internal/cmd/firewall/firewall.go 0.00% 11 Missing ⚠️
internal/cmd/util/util.go 0.00% 9 Missing ⚠️
internal/cmd/image/image.go 0.00% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #675      +/-   ##
==========================================
- Coverage   45.32%   44.76%   -0.56%     
==========================================
  Files         179      179              
  Lines        7855     7952      +97     
==========================================
  Hits         3560     3560              
- Misses       3842     3939      +97     
  Partials      453      453              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@phm07 phm07 force-pushed the command-groups branch 2 times, most recently from c5ffb64 to 321842f Compare January 31, 2024 09:27
@phm07 phm07 marked this pull request as ready for review January 31, 2024 10:34
@phm07 phm07 requested a review from a team as a code owner January 31, 2024 10:34
Copy link
Member

@apricote apricote left a comment

Choose a reason for hiding this comment

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

Very nice documentation! Props to you.

Found some inconsistencies and missed subcommands.

CONTRIBUTING.md Outdated Show resolved Hide resolved
internal/cmd/loadbalancer/load_balancer.go Outdated Show resolved Hide resolved
internal/cmd/network/network.go Outdated Show resolved Hide resolved
internal/cmd/primaryip/primaryip.go Outdated Show resolved Hide resolved
internal/cmd/primaryip/primaryip.go Outdated Show resolved Hide resolved
internal/cmd/volume/volume.go Outdated Show resolved Hide resolved
@apricote
Copy link
Member

What do you think about adding groups to the root command?

	rootCommand.AddGroup(&cobra.Group{ID: "resources", Title: "Resources:"})

	rootCommand.AddCommand(
		util.WithGroup("resources", all.NewCommand(s)),
		util.WithGroup("resources", floatingip.NewCommand(s)),
		util.WithGroup("resources", image.NewCommand(s)),
		util.WithGroup("resources", server.NewCommand(s)),
		util.WithGroup("resources", sshkey.NewCommand(s)),
		util.WithGroup("resources", servertype.NewCommand(s)),
		util.WithGroup("resources", datacenter.NewCommand(s)),
		util.WithGroup("resources", location.NewCommand(s)),
		util.WithGroup("resources", iso.NewCommand(s)),
		util.WithGroup("resources", volume.NewCommand(s)),
		util.WithGroup("resources", network.NewCommand(s)),
		util.WithGroup("resources", loadbalancer.NewCommand(s)),
		util.WithGroup("resources", loadbalancertype.NewCommand(s)),
		util.WithGroup("resources", certificate.NewCommand(s)),
		util.WithGroup("resources", firewall.NewCommand(s)),
		util.WithGroup("resources", placementgroup.NewCommand(s)),
		util.WithGroup("resources", primaryip.NewCommand(s)),

		util.WithGroup("", version.NewCommand(s)),
		util.WithGroup("", completion.NewCommand(s)),
		util.WithGroup("", context.NewCommand(s)),
	)
Before & After

Before

A command-line interface for Hetzner Cloud

Usage:
  hcloud [command]

Available Commands:
  all                Commands that apply to all resources
  certificate        Manage certificates
  completion         Output shell completion code for the specified shell
  context            Manage contexts
  datacenter         Manage datacenters
  firewall           Manage Firewalls
  floating-ip        Manage Floating IPs
  help               Help about any command
  image              Manage images
  iso                Manage ISOs
  load-balancer      Manage Load Balancers
  load-balancer-type Manage Load Balancer types
  location           Manage locations
  network            Manage networks
  placement-group    Manage Placement Groups
  primary-ip         Manage Primary IPs
  server             Manage servers
  server-type        Manage server types
  ssh-key            Manage SSH keys
  version            Print version information
  volume             Manage Volumes

Flags:
  -h, --help                     help for hcloud
      --poll-interval duration   Interval at which to poll information, for example action progress (default 500ms)
      --quiet                    Only print error messages

Use "hcloud [command] --help" for more information about a command.

After

A command-line interface for Hetzner Cloud

Usage:
  hcloud [command]

Resources:
  all                Commands that apply to all resources
  certificate        Manage certificates
  datacenter         Manage datacenters
  firewall           Manage Firewalls
  floating-ip        Manage Floating IPs
  image              Manage images
  iso                Manage ISOs
  load-balancer      Manage Load Balancers
  load-balancer-type Manage Load Balancer types
  location           Manage locations
  network            Manage networks
  placement-group    Manage Placement Groups
  primary-ip         Manage Primary IPs
  server             Manage servers
  server-type        Manage server types
  ssh-key            Manage SSH keys
  volume             Manage Volumes

Additional Commands:
  completion         Output shell completion code for the specified shell
  context            Manage contexts
  help               Help about any command
  version            Print version information

Flags:
  -h, --help                     help for hcloud
      --poll-interval duration   Interval at which to poll information, for example action progress (default 500ms)
      --quiet                    Only print error messages

Use "hcloud [command] --help" for more information about a command.

Copy link
Member

@jooola jooola left a comment

Choose a reason for hiding this comment

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

I think its awesome!

Just wanted to ask whether we prefer to use a manual sorting for the commands ?
Right now the command are sorted alphabetically, I think it will make more sense if we keep our own ordering.

I believe @phm07 was against this idea, @apricote what do you think?

Manage servers

Usage:
  hcloud server [command]

General
  list                        List Servers
  describe                    Describe a server
  create                      Create a server
  delete                      Delete a server
  update                      Update a Server
  create-image                Create an image from a server
  change-type                 Change type of a server
  rebuild                     Rebuild a server
  add-label                   Add a label to a server
  remove-label                Remove a label from a server

Protection
  enable-protection           Enable resource protection for a server
  disable-protection          Disable resource protection for a server

Rescue
  enable-rescue               Enable rescue for a server
  disable-rescue              Disable rescue for a server

Power/Reboot
  poweron                     Poweron a server
  poweroff                    Poweroff a server
  reboot                      Reboot a server
  shutdown                    Shutdown a server
  reset                       Reset a server

Networks
  attach-to-network           Attach a server to a network
  detach-from-network         Detach a server from a network
  change-alias-ips            Change a server's alias IPs in a network
  set-rdns                    Change reverse DNS of a Server

ISO
  attach-iso                  Attach an ISO to a server
  detach-iso                  Detach an ISO from a server

Placement Groups
  add-to-placement-group      Add a server to a placement group
  remove-from-placement-group Removes a server from a placement group

Backup
  enable-backup               Enable backup for a server
  disable-backup              Disable backup for a server

Additional Commands:
  ssh                         Spawn an SSH connection for the server
  ip                          Print a server's IP address
  request-console             Request a WebSocket VNC console for a server
  reset-password              Reset the root password of a server
  metrics                     [ALPHA] Metrics from a Server

Flags:
  -h, --help   help for server

Global Flags:
      --poll-interval duration   Interval at which to poll information, for example action progress (default 500ms)
      --quiet                    Only print error messages

Use "hcloud server [command] --help" for more information about a command.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
internal/cmd/loadbalancer/load_balancer.go Outdated Show resolved Hide resolved
internal/cmd/util/util.go Outdated Show resolved Hide resolved
@phm07
Copy link
Contributor Author

phm07 commented Jan 31, 2024

@apricote I like the idea of adding groups to the root command. I'll add that in.

@jooola We can already change the order of the groups, and inside the groups the commands are alphabetically sorted. I like that because it enforces consistent order inside of groups (for example for the General category, where commands are most often the same). Also, if you want to reorder commands inside of a group, that is probably a sign that they should go into their own group.

@jooola
Copy link
Member

jooola commented Jan 31, 2024

@jooola We can already change the order of the groups, and inside the groups the commands are alphabetically sorted. I like that because it enforces consistent order inside of groups (for example for the General category, where commands are most often the same). Also, if you want to reorder commands inside of a group, that is probably a sign that they should go into their own group.

I agree that consitency is a good thing, but if the ordering doesn't make much sens, maybe it worth perfecting the order by hand, for example the server general section:

# Before
General
  add-label                   Add a label to a server
  change-type                 Change type of a server
  create                      Create a server
  create-image                Create an image from a server
  delete                      Delete a server
  describe                    Describe a server
  list                        List Servers
  rebuild                     Rebuild a server
  remove-label                Remove a label from a server
  update                      Update a Server

# After
General
  list                        List Servers
  describe                    Describe a server
  create                      Create a server
  delete                      Delete a server
  update                      Update a Server
  add-label                   Add a label to a server
  remove-label                Remove a label from a server
  create-image                Create an image from a server
  change-type                 Change type of a server
  rebuild                     Rebuild a server

@apricote
Copy link
Member

Sooo, that means labels should be their own group? :D

Its the one example that I found pretty annoying right now, because they are at the top and bottom of "General".

I do agree that keeping it manually sorted is pretty annoying, but from a user perspective it looks way nicer (as long as that sorting is consistent between the commands).

@phm07
Copy link
Contributor Author

phm07 commented Jan 31, 2024

I'm not opposed to making the label commands have their own group

@jooola
Copy link
Member

jooola commented Jan 31, 2024

I do agree that keeping it manually sorted is pretty annoying, but from a user perspective it looks way nicer (as long as that sorting is consistent between the commands).

I think that's the kind of work we have to do once, and its done.

But I won't argue more than that.

(to disable the sorting, you can set cobra.EnableCommandSorting = false 😉 )

We could also sort the root commands by importance!

Copy link
Member

@jooola jooola left a comment

Choose a reason for hiding this comment

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

Awesome!

@phm07 phm07 merged commit 0cb271f into main Feb 1, 2024
3 checks passed
@phm07 phm07 deleted the command-groups branch February 1, 2024 11:05
apricote pushed a commit that referenced this pull request Feb 1, 2024
## [1.42.0](v1.41.1...v1.42.0) (2024-02-01)


### Features

* add global --quiet flag to hide non-error messages
([#656](#656))
([25fcbbf](25fcbbf)),
closes [#644](#644)
* allow adding/removing multiple labels at once
([#665](#665))
([919c446](919c446)),
closes [#662](#662)
* group subcommands in command help
([#675](#675))
([0cb271f](0cb271f))
* **server:** remove unsupported linux32 rescue type
([#679](#679))
([5bb0350](5bb0350))


### Bug Fixes

* refetch after creating managed certificate
([#685](#685))
([4864553](4864553))
* **server:** fix typo in ip subcommand
([#678](#678))
([c5e3f00](c5e3f00))
* use --poll-interval flag
([#660](#660))
([b9328a6](b9328a6))
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