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

Enforce consistent args rejection for no-args commands: ls, restart, version #9660

Merged
merged 4 commits into from
Jul 29, 2022

Conversation

piroux
Copy link
Contributor

@piroux piroux commented Jul 16, 2022

What I did
Make cobra to reject arguments for no-args commands in the same manner:

  • ls
  • version
  • (down was done already)

For the restart command, the documentation is updated to let the user know that services can be passed as arguments.

Related issue

Based on #9158 which enforced it for the down command.


According to the listing below, these 4 commands were the only ones to not handle arguments.
➜ for c in (docker compose --help | awk '/^  [a-z]+ +.*/ {print $1}');
      docker compose $c --help | grep 'Usage:';
  end | cat
Usage:  docker compose build [SERVICE...]
Usage:  docker compose convert SERVICES
Usage:  docker compose cp [OPTIONS] SERVICE:SRC_PATH DEST_PATH|-
Usage:  docker compose create [SERVICE...]
Usage:  docker compose down
Usage:  docker compose events [options] [--] [SERVICE...]
Usage:  docker compose exec [options] [-e KEY=VAL...] [--] SERVICE COMMAND [ARGS...]
Usage:  docker compose images [SERVICE...]
Usage:  docker compose kill [options] [SERVICE...]
Usage:  docker compose logs [SERVICE...]
Usage:  docker compose ls
Usage:  docker compose pause [SERVICE...]
Usage:  docker compose port [options] [--] SERVICE PRIVATE_PORT
Usage:  docker compose ps [SERVICE...]
Usage:  docker compose pull [SERVICE...]
Usage:  docker compose push [SERVICE...]
Usage:  docker compose restart
Usage:  docker compose rm [SERVICE...]
Usage:  docker compose run [options] [-v VOLUME...] [-p PORT...] [-e KEY=VAL...] [-l KEY=VALUE...] SERVICE [COMMAND] [ARGS...]
Usage:  docker compose start [SERVICE...]
Usage:  docker compose stop [SERVICE...]
Usage:  docker compose top [SERVICES...]
Usage:  docker compose unpause [SERVICE...]
Usage:  docker compose up [SERVICE...]
Usage:  docker compose version

Copy link
Contributor

@ulyssessouza ulyssessouza left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@glours glours left a comment

Choose a reason for hiding this comment

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

I let you choose what you want to do:

  • Update the restart documentation in your PR
  • Remove your changes for the restart command

@@ -40,6 +40,7 @@ func restartCommand(p *projectOptions, backend api.Service) *cobra.Command {
RunE: Adapt(func(ctx context.Context, args []string) error {
return runRestart(ctx, backend, opts, args)
}),
Args: cobra.NoArgs,
Copy link
Contributor

@glours glours Jul 26, 2022

Choose a reason for hiding this comment

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

Ok I think we should update the documentation of the restart command to show the optional service name arg!
Restart allows you to choose a specific service to restart so you can pass an argument like that:

docker compose restart my-service

Copy link
Member

@laurazard laurazard Jul 26, 2022

Choose a reason for hiding this comment

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

We shouldn't change this, right? It implies losing the ability to just restart a specific service, which people might be relying on. I'd prefer to keep the current compose restart behaviour unless there is a reason for changing it.

Copy link
Contributor

@glours glours Jul 26, 2022

Choose a reason for hiding this comment

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

We'll keep the actual behavior for sure, that's why I proposed to update the doc or remove the modification for the restart command

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I had overlooked that restart could be used like that too.

So yes, let's update the docs.

@glours
Copy link
Contributor

glours commented Jul 28, 2022

You need to resolve your conflict in order to start the CI process

Copy link
Contributor

@glours glours left a comment

Choose a reason for hiding this comment

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

Doing modification in the documentation isn't trivial 😅
You should modify the command itself in the restart.go file and then use make docs to generate the yaml and md doc files.

docs/reference/compose_restart.md Show resolved Hide resolved
docs/reference/docker_compose_restart.yaml Show resolved Hide resolved
docs/reference/docker_compose_restart.yaml Show resolved Hide resolved
docs/reference/docker_compose_restart.yaml Show resolved Hide resolved
docs/reference/compose_restart.md Show resolved Hide resolved
@piroux piroux force-pushed the consistent-noargs branch 2 times, most recently from 737abf9 to 341e7d0 Compare July 29, 2022 09:33
Copy link
Contributor

@glours glours left a comment

Choose a reason for hiding this comment

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

LGTM

@glours glours requested a review from ulyssessouza July 29, 2022 13:16
@milas milas merged commit 38a1879 into docker:v2 Jul 29, 2022
@piroux piroux deleted the consistent-noargs branch August 3, 2022 08:42
@piroux piroux restored the consistent-noargs branch August 3, 2022 08:42
@piroux piroux deleted the consistent-noargs branch August 3, 2022 08:43
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.

5 participants