Skip to content

Conversation

nalind
Copy link
Member

@nalind nalind commented Jun 4, 2025

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Get out of business of manually crafting and updating shell completion helper functions.
Fixes #6069

How to verify it

I don't use a shell that benefits from this (old habit), but if you do, source the new file and see if it works?

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Shell completions are now generated rather than manually crafted, so completion suggestions will look different.

@openshift-ci openshift-ci bot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. approved labels Jun 4, 2025
Copy link
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

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

LGTM @Luap99 might want to take a quick look at this one.

Copy link
Contributor

openshift-ci bot commented Jun 4, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, nalind

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

I have not tested it yet but I assume we would like to add some autocompletion functions to auto complete container/image names at least for the basic commands.

Otherwise we regress users compared to the existing bash script.

Makefile Outdated

.PHONY: install.completions
install.completions:
install.completions: contrib/completions/bash/buildah
Copy link
Member

Choose a reason for hiding this comment

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

this should install for zsh and fish as well, see the podman Makefile for an example

Copy link
Member Author

Choose a reason for hiding this comment

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

Adding.

Copy link
Member

Choose a reason for hiding this comment

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

In hindsight I think committing the files in podman was a mistake, every cobra update that changes the files will means you need to regenerate so that auto renovate updates won't work.

So in short I don't think we should commit these completion files at all since the build process is generating them anyway,

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough. I'll drop them.

@Luap99
Copy link
Member

Luap99 commented Jun 4, 2025

Also I guess this should have a Fixes #6069?

@Luap99
Copy link
Member

Luap99 commented Jun 4, 2025

Ok I did some local tests I think we might one to port some other changes from podman such as
https://github.com/containers/podman/blob/badf6b8b1735dff7cdb45636eb8eba490454f9d9/cmd/podman/root.go#L311-L317

$ bin/buildah rm -- 
--all                    (remove all containers)
--cgroup-manager         (cgroup manager)
--help                   (help for rm)
--log-level              (the log level to be used, one of "trace", "debug", "info", "warn", "error", "fatal", or "panic")
--registries-conf-dir    (path to registries.conf.d directory (not usually used))
--registries-conf        (path to registries.conf file (not usually used))
--root                   (storage root dir)
--runroot                (storage state dir)
--short-name-alias-conf  (path to short name alias cache file (not usually used))
--storage-driver         (storage-driver)
--storage-opt            (storage driver option)
--userns-gid-map         (default `ctrID:hostID:length` GID mapping to use)
--userns-uid-map         (default `ctrID:hostID:length` UID mapping to use)

Because the are persistent flags they can be set technically anywhere but realistically for a user suggesting them on each command makes no sense.
Second as I though just doing buildah rmi no longer completes the image ids/names which is a clear UX regression so I rather see that implemented before we switch the script.

I am happy to take that over if you don't want to spend the time understanding all the small details in play here, its something I can do while being offline on the train next week.

@nalind
Copy link
Member Author

nalind commented Jun 4, 2025

I am happy to take that over if you don't want to spend the time understanding all the small details in play here, its something I can do while being offline on the train next week.

If you want to, sure, otherwise I could stand to get more familiar with those bits.

Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

Get out of business of manually crafting and updating shell completion
helper functions.

Signed-off-by: Nalin Dahyabhai <[email protected]>
@nalind
Copy link
Member Author

nalind commented Jun 4, 2025

/retitle WIP: switch to automatically-generated completions

@openshift-ci openshift-ci bot changed the title Switch to automatically-generated completions WIP: switch to automatically-generated completions Jun 4, 2025
@Luap99
Copy link
Member

Luap99 commented Jun 4, 2025

If you want to, sure, otherwise I could stand to get more familiar with those bits.

I find other stuff to do, if you want to have some deeper understanding please do that.

https://github.com/containers/podman/blob/badf6b8b1735dff7cdb45636eb8eba490454f9d9/cmd/podman/root.go#L302-L321
This must be done in persistentPreRunE() function that is registered on the root command here in buildah in a similar way, I see there is already a before() function where it could be added. First it is used to hide the top level flags on subcommand so they don't show up all the time. The cmd.Root().Traverse(os.Args[2:]) call is not right in the context of buildah I think, podman has some really special cli setup where this hack was needed but I don't know if it is needed for buildah. Traverse() is definly wrong because that must only be used when TraverseChildren is set to true which it is not for buildah so it would need to be Find() if it is needed at all.
I guess the easier way to check if the command being pass in is the root buildah command is to just compare the pointer of cmd against rootCmd.

For actual specific completion suggestions there is https://github.com/containers/podman/blob/badf6b8b1735dff7cdb45636eb8eba490454f9d9/cmd/podman/common/completion.go with a ton of example functions.

Than you just have to register them to the command via ValidArgsFunction or in case of flags RegisterFlagCompletionFunc(),

@Luap99
Copy link
Member

Luap99 commented Jun 4, 2025

And for testing with bash source <(bin/buildah completion bash) will work.

The nice thing about the binary is generating the completions is if you build buildah from source and let's say added a new command then it will work right away without having to reload the script, bin/buildah [TAB] would show it while /usr/bin/buildah [TAB] will not since that binary does have the command yet.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 25, 2025
@openshift-merge-robot
Copy link
Collaborator

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Copy link

A friendly reminder that this PR had no activity for 30 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved do-not-merge/work-in-progress kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. stale-pr
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bash completion error with shopt -s failglob
4 participants