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

Introduce support for droplet autoscaler #1606

Merged
merged 8 commits into from
Nov 15, 2024

Conversation

asaha2
Copy link
Contributor

@asaha2 asaha2 commented Oct 31, 2024

Droplet autoscaler is currently in closed alpha and enabled for select customer.

Nit change also includes marking the new load balancer feature flags as visible in the cli text (now that these features are available since GA).

TODO:

  • test compiled binary
  • add unit tests

@asaha2
Copy link
Contributor Author

asaha2 commented Nov 1, 2024

cc @andrewsomething when you have a chance 🙏

@asaha2 asaha2 force-pushed the asaha/AUTOSCALE-53 branch from bba410c to 805b1f7 Compare November 6, 2024 15:52
Copy link
Member

@andrewsomething andrewsomething left a comment

Choose a reason for hiding this comment

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

Overall this looks great. There are some minor copy suggestions inline.

The one issue I'm seeing is with the Displayable implementation. Rather than using a single type with switch statements, each should be its own type. This will allow you to remove the format flags that have been added manually. When the displayer is implemented correctly, the flag is added automatically. The help output will also have the column names populated automatically.

It would also be great to get some additional help information into the command long descriptions, but that can follow later. If there is someone on the product docs team working on this release, they might be able help there.

commands/displayers/droplet_autoscale.go Outdated Show resolved Hide resolved
commands/droplet_autoscale.go Outdated Show resolved Hide resolved
commands/droplet_autoscale.go Outdated Show resolved Hide resolved
commands/droplet_autoscale.go Outdated Show resolved Hide resolved
commands/droplet_autoscale.go Outdated Show resolved Hide resolved
commands/droplet_autoscale.go Outdated Show resolved Hide resolved
commands/droplet_autoscale.go Outdated Show resolved Hide resolved
commands/droplet_autoscale.go Outdated Show resolved Hide resolved
commands/droplet_autoscale.go Outdated Show resolved Hide resolved
commands/displayers/droplet_autoscale.go Outdated Show resolved Hide resolved
Copy link
Member

@andrewsomething andrewsomething left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! This is very close. Though I did notice that you changed the commands that require an autoscale pool ID from using a positional argument to an --id flag. Any particular reason for that?

Most doctl commands that act on an existing resource tag the ID as a positional argument. For example: doctl compute droplet get <id> Unless there is a specific reason not too, it would be more consistent to do so here too.

@asaha2
Copy link
Contributor Author

asaha2 commented Nov 12, 2024

@andrewsomething Yeah should've perhaps checked in with you on this: it just seemed counter-intuitive that we accept --id as per command help text, but if we run it with --id=<id> we seem to get a Error: (droplet-autoscale.list-members) command is missing required arguments E.g.:

asaha@C02FLDHYMD6M:doctl ./builds/doctl -t $DIGITALOCEAN_TOKEN -u https://api.s2r1.internal.digitalocean.com compute droplet-autoscale list-members -h
List all members of a Droplet autoscale pool

Usage:
  doctl compute droplet-autoscale list-members [flags]

Flags:
      --format ID   Columns for output in a comma-separated list. Possible values: ID, `Status`, `Health Status`, `Unhealthy Reason`, `CPU Util`, `Mem Util`.
  -h, --help        help for list-members
      --id string   ID of the Droplet autoscale pool (required)
      --no-header   Return raw data with no headers
asaha@C02FLDHYMD6M:doctl ./builds/doctl -t $DIGITALOCEAN_TOKEN -u https://api.s2r1.internal.digitalocean.com compute droplet-autoscale list-members --id=41fd76b0-3487-4ae6-8979-d26da4b7b122
Error: (droplet-autoscale.list-members) command is missing required arguments
asaha@C02FLDHYMD6M:doctl ./builds/doctl -t $DIGITALOCEAN_TOKEN -u https://api.s2r1.internal.digitalocean.com compute droplet-autoscale list-members 41fd76b0-3487-4ae6-8979-d26da4b7b122
ID          Status    Health Status    Unhealthy Reason    CPU Util    Mem Util
11735085    active    healthy                              0.001083    0.366714
11735086    active    healthy                              0.000704    0.382897

Copy link
Member

@andrewsomething andrewsomething left a comment

Choose a reason for hiding this comment

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

👍 LGTM!

@andrewsomething andrewsomething merged commit fbf093f into digitalocean:main Nov 15, 2024
8 checks passed
@asaha2 asaha2 deleted the asaha/AUTOSCALE-53 branch November 15, 2024 20:28
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.

2 participants