Skip to content

New apps:scale option format#224

Merged
krallin merged 1 commit intoaptible:masterfrom
krallin:no-operation-container-count-default
May 12, 2017
Merged

New apps:scale option format#224
krallin merged 1 commit intoaptible:masterfrom
krallin:no-operation-container-count-default

Conversation

@krallin
Copy link
Contributor

@krallin krallin commented May 3, 2017

This changes the aptible apps:scale option format to treat container
count and container size as two "equal" options, rather than always
require container count to be provided as a positional argument and
allow container size to be provided as an afterthought.

To do so, both options are now positional arguments. I have however also
renamed one of the options: --size is now --container-size.

The rationale here is that we also use --size for db:create and
backup:restore in order to reference the size of the disk, but when we
introduce self-service DB scaling, we're going to want to also allow
specifying the container size, and that parameter will have to be named
differently (presumably --container-size).

In other words, for the sake of consistency, I'm updating apps:scale
to use --container- prefixes in the options so that the DB operations
can use the same parameters going forward.

Now, all these changes are potentially breaking changes, which we'd
rather avoid, so I've made sure to continue supporting the legacy
command line options (I did refactor most tests we had here, but I
confirmed the old tests were passing with the new code beforehand, and
I've kept tests for the old behavior around).

Finally, as we discussed, I also removed the size enum from the
--container-size option, since API will now be providing the list of
valid values if the user tries to use an unsupported size. This ensures
we won't need new CLI releases if new sizes are made available.


cc @fancyremarker

This changes the aptible apps:scale option format to treat container
count and container size as two "equal" options, rather than always
require container count to be provided as a positional argument and
allow container size to be provided as an afterthought.

To do so, both options are now positional arguments. I have however also
*renamed* one of the options: `--size` is now `--container-size`.

The rationale here is that we also use `--size` for `db:create` and
`backup:restore` in order to reference the size of the disk, but when we
introduce self-service DB scaling, we're going to want to also allow
specifying the container size, and that parameter will have to be named
differently (presumably `--container-size`).

In other words, for the sake of consistency, I'm updating `apps:scale`
to use `--container-` prefixes in the options so that the DB operations
can use the same parameters going forward.

Now, all these changes are potentially breaking changes, which we'd
rather avoid, so I've made sure to continue supporting the legacy
command line options (I did refactor most tests we had here, but I
confirmed the old tests were passing with the new code beforehand, and
I've kept tests for the old behavior around).

Finally, as we discussed, I also removed the size enum from the
`--container-size` option, since API will now be providing the list of
valid values if the user tries to use an unsupported size. This ensures
we won't need new CLI releases if new sizes are made available.
@krallin krallin force-pushed the no-operation-container-count-default branch from fcb6bfa to 761a181 Compare May 3, 2017 11:34
@codecov-io
Copy link

codecov-io commented May 3, 2017

Codecov Report

Merging #224 into master will increase coverage by 0.26%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #224      +/-   ##
==========================================
+ Coverage   91.31%   91.58%   +0.26%     
==========================================
  Files          44       44              
  Lines        1532     1580      +48     
==========================================
+ Hits         1399     1447      +48     
  Misses        133      133
Impacted Files Coverage Δ
spec/aptible/cli/subcommands/apps_spec.rb 100% <100%> (ø) ⬆️
lib/aptible/cli/subcommands/apps.rb 80% <100%> (+10.43%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c602184...761a181. Read the comment docs.

@krallin
Copy link
Contributor Author

krallin commented May 3, 2017

@fancyremarker (and @UserNotFound) as a side note, I wanted to discuss the description we provide for commands. For most commands, we don't show the various flags available in the aptible help text, so you can only see these options when you use aptible help SOME_COMMAND.

For comparison, here are two lines from the generic aptible help:

aptible apps:scale SERVICE [--container-count COUNT] [--container-size SIZE]  # Scale a service

aptible db:create HANDLE                                                      # Create a new database

But unlike what's reported there, both commands accept various flags:

Usage:
  aptible apps:scale SERVICE [--container-count COUNT] [--container-size SIZE]

Options:
      [--app=APP]
      [--environment=ENVIRONMENT]
  -r, [--remote=REMOTE]
      [--container-count=N]
      [--container-size=N]
      [--size=N]                   # DEPRECATED, use --container-size

Scale a service
Usage:
  aptible db:create HANDLE

Options:
  [--type=TYPE]
                               # Default: postgresql
  [--size=N]
                               # Default: 10
  [--environment=ENVIRONMENT]

Create a new database

There are a few things we could do here:

  1. Go with the Thor default. Don't show options unless they're required. For options like apps:scale, this will suck because it means what you're shown in the generic help text isn't sufficient to use the command
  2. Do it on a case-by-case basis. That's what we do right now. I feel like it's not ideal because when you see some options in the list an end-user may assume that a) these are all the options that exist (which is incorrect: for apps:scale we're missing --environment and --app), and that b) commands without options don't have any (which is also incorrect: for db:create you probably want to consider the --type option before creating a DB...).
  3. Always show all options. Personally I favor this option, but it might be a little difficult to get that to work with Thor (I intuitively feel like this is a problem a CLI toolkit should be handling, but unfortunately unless I'm missing something, that's not actually the case with Thor).

Thoughts?

Copy link
Member

@fancyremarker fancyremarker left a comment

Choose a reason for hiding this comment

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

Thank you!

@UserNotFound
Copy link
Member

I like option 3, too, though it sounds like there's not a canonical way to accomplish it with Thor? It might be possible to have a longer help text defined that contains the broader options (that might apply to all app: or db: commands) and include that in a long_desc for each command . I believe would show just before Usage: and Options: in the help output?

At the very least, maybe outline those 'global' options at the top of the output of aptible help, if there's not a DRY way to include it in aptible help [COMMAND]?

@krallin krallin merged commit 5fe519f into aptible:master May 12, 2017
@krallin
Copy link
Contributor Author

krallin commented May 12, 2017

Thanks @UserNotFound! I'll open a new issue so we don't lose track of this conversation now that this is merged.

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.

4 participants