Skip to content

Conversation

thaJeztah
Copy link
Member

cli: disable file-completion by default

This uses the DefaultShellCompDirective feature which was added
in cobra to override the default (which would complete to use
files for commands and flags).

cli-plugins: disable file-completion by default

This uses the DefaultShellCompDirective feature which was added
in cobra to override the default (which would complete to use
files for commands and flags).

- How I did it

- How to verify it

- Human readable description for the release notes

- A picture of a cute animal (not mandatory but encouraged)

This uses the DefaultShellCompDirective feature which was added
in cobra to override the default (which would complete to use
files for commands and flags).

Note that we set "cobra.NoFileCompletions" for many commands, which
is redundant with this change, so we could remove as well.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
This uses the DefaultShellCompDirective feature which was added
in cobra to override the default (which would complete to use
files for commands and flags).

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah added this to the 29.0.0 milestone Sep 10, 2025
@thaJeztah thaJeztah added status/2-code-review area/completion kind/refactor PR's that refactor, or clean-up code labels Sep 10, 2025
@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
cli-plugins/plugin/plugin.go 0.00% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

@thaJeztah
Copy link
Member Author

FWIW; this would allow us to remove uses of cobra.NoFileCompletions from command definitions, but I thought it was better to keep them to more clearly indicate "this is disabled by choice";

ValidArgsFunction: cobra.NoFileCompletions,

I also considered adding a completion.TODO alias for that one, similar to context.TODO for places where we currently disable completion, but because it still has to be implemented.

@thaJeztah
Copy link
Member Author

cc @crazy-max @ndeloof in case buildx or compose depends on the behavior to complete with filenames (for both commands and flags); I put the "disable for plugins" as a separate commit.

@thaJeztah thaJeztah requested a review from Benehiko September 10, 2025 12:30
@thaJeztah thaJeztah merged commit c3ceba2 into docker:master Sep 10, 2025
99 checks passed
@thaJeztah thaJeztah deleted the default_completion branch September 10, 2025 13:07
@crazy-max
Copy link
Member

crazy-max commented Sep 11, 2025

cc @crazy-max @ndeloof in case buildx or compose depends on the behavior to complete with filenames (for both commands and flags); I put the "disable for plugins" as a separate commit.

@thaJeztah For flags/cmds that need file completion in cli plugins I guess it needs follow-ups to opt-in with this implementation?

@thaJeztah
Copy link
Member Author

Yes, probably; we can try when I update my PR that vendors the latest from master.

Currently the default is that any flag and any command that doesn't have completion configured will default to completing based on the filesystem; in MOST cases that's not desirable; for example, with buildx;

docker buildx build --platform<tab>
AUTHORS   codecov.yml      docs/    LICENSE      README.md  version/
bake/     commands/        driver/  localstate/  store/
bin/      controller/      foo/     MAINTAINERS  tests/
...

With this option set, the above will just "not provide completion", but of course some commands do want completion for files (or directories, or files with a specific extension), so for those, it will have to be explicitly enabled.

FWIW, I was considering adding a completion.TODO completion that would be the equivalent of "no (file) completion", similar to context.TODO() - that option would be for cases where we know completion is missing, and still has to be added, making it easier to find back places where work is still to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants