Transport completions #2524

yedayak · 2025-02-17T21:01:49Z

Another thing I would like to add is completion of containers-storage:, but I'm not sure how to get the list of available images.

Signed-off-by: Yedaya Katsman <yedaya.ka@gmail.com>

Luap99

Overall this looks like a nice improvement, just some comments on the implementation. (Note I am not an official maintainer here but I have done most of the cobra completion work in podman and skopeo)

Luap99 · 2025-02-27T10:44:41Z

cmd/skopeo/completions.go

+func autocompleteTransports(cmd *cobra.Command, args []string, toComplete string) ([]cobra.Completion, cobra.ShellCompDirective) {
+	directive := cobra.ShellCompDirectiveNoSpace
+	// We still don't have a transport
+	if !strings.Contains(toComplete, ":") {


Overall this would likely be much better written with strings.Cut()

That way you can easily check if a colon was set and the you have the clear prefix and can match that without the colon below.

Yeah, this works much better. Updated to do this

Luap99 · 2025-02-27T10:46:39Z

cmd/skopeo/completions.go

+	if strings.HasPrefix(toComplete, "oci-archive:") || strings.HasPrefix(toComplete, "docker-archive:") || strings.HasPrefix(toComplete, "sif:") || strings.HasPrefix(toComplete, "oci:") {
+		return nil, directive
+	}
+	if toComplete == "docker:" {
+		return []cobra.Completion{"docker://"}, directive | cobra.ShellCompDirectiveNoFileComp
+	}
+	if strings.HasPrefix(toComplete, "dir:") {


here you then should match the transpart names as defined by the code and not hard code the strings
docker.Transport.Name(), directory.Transport.Name(),etc...

Luap99 · 2025-02-27T10:50:12Z

cmd/skopeo/completions.go

+		// just ShellCompDirectiveFilterDirs is more correct, but doesn't work here in bash, see https://github.com/spf13/cobra/issues/2242. Instead we get the directories ourselves.
+		curDir := filepath.Dir(strings.TrimPrefix(toComplete, "dir:"))
+		entries, err := os.ReadDir(curDir)
+		if err != nil {
+			cobra.CompErrorln("Failed ReadDir at " + curDir)
+			// Fallback to whatever the shell gives us
+			return nil, cobra.ShellCompDirectiveFilterDirs | cobra.ShellCompDirectiveNoFileComp
+		}
+		suggestions := make([]cobra.Completion, 0, len(entries))
+		for _, e := range entries {
+			if e.IsDir() {
+				suggestions = append(suggestions, "dir:"+path.Join(curDir, e.Name())+"/")
+			}
+		}
+		return suggestions, cobra.ShellCompDirectiveNoFileComp | cobra.ShellCompDirectiveNoSpace


Is that actually worth it? Returning all paths as done by the shell seems more than fine, compared of having to read all files here.

I agree its a bit overkill, this just makes the completions more accurate, showing only directories. I'm fine with not doing it.

packit-as-a-service · 2025-02-27T14:41:08Z

Tests failed. @containers/packit-build please check.

lsm5 · 2025-02-27T14:56:55Z

@Luap99 do we test completions in CI anywhere yet?

Luap99 · 2025-02-27T15:12:23Z

@Luap99 do we test completions in CI anywhere yet?

In podman we have number of rather complex tests (test/system/600-completion.bats). But that is not something I would recommend for skopeo, most of the changes here are trivial to test manually (and it is not like they will get changed often) and there is basically no risk if the completion is not working fully.

mtrmac

Thanks!

cmd/skopeo/completions.go

mtrmac · 2025-02-27T19:50:00Z

cmd/skopeo/completions.go

-// autocompleteSupportedTransports list all supported transports with the colon suffix.
-func autocompleteSupportedTransports(cmd *cobra.Command, args []string, toComplete string) ([]cobra.Completion, cobra.ShellCompDirective) {
+func autocompleteTransports(cmd *cobra.Command, args []string, toComplete string) ([]cobra.Completion, cobra.ShellCompDirective) {
+	directive := cobra.ShellCompDirectiveNoSpace


I don’t see a benefit to using this variable; AFAICS it only makes the return values harder to understand.

Yeah, I over complicated it :) removed

mtrmac · 2025-02-27T20:11:38Z

cmd/skopeo/completions.go

+		if err != nil {
+			cobra.CompErrorln("Failed ReadDir at " + curDir)
+			// Fallback to whatever the shell gives us
+			return nil, cobra.ShellCompDirectiveFilterDirs | cobra.ShellCompDirectiveNoFileComp


The way I read the documentation, NoFileComp should make FilterDirs entirely irrelevant. (In the implementation, that seems not to matter.)

Yeah ShellCompDirectiveFilterDirs by itself should just complete directories (though in bash it currently doesn't). fixed

cmd/skopeo/completions.go

mtrmac · 2025-02-27T20:20:05Z

cmd/skopeo/completions.go

 )

-// autocompleteSupportedTransports list all supported transports with the colon suffix.
-func autocompleteSupportedTransports(cmd *cobra.Command, args []string, toComplete string) ([]cobra.Completion, cobra.ShellCompDirective) {
+func autocompleteTransports(cmd *cobra.Command, args []string, toComplete string) ([]cobra.Completion, cobra.ShellCompDirective) {


This is now autocompleteImageNames, now that it is trying to complete the full arguments.

mtrmac · 2025-02-27T20:20:52Z

cmd/skopeo/completions.go

-func autocompleteSupportedTransports(cmd *cobra.Command, args []string, toComplete string) ([]cobra.Completion, cobra.ShellCompDirective) {
+func autocompleteTransports(cmd *cobra.Command, args []string, toComplete string) ([]cobra.Completion, cobra.ShellCompDirective) {
+	directive := cobra.ShellCompDirectiveNoSpace
+	// We still don't have a transport


This comment belongs inside the next if (or on the if !haveTransport line), it is incorrect for other paths through this function.

Removed the comment since it's now obvious from the variable name

mtrmac · 2025-02-27T20:25:34Z

@Luap99 do we test completions in CI anywhere yet?

In podman we have number of rather complex tests (test/system/600-completion.bats).

Here, I think it would be easier, and we would not give up on ~any test coverage compared to what Podman does, to use Go unit tests for the individual parts (and, for that purpose, to split the dir: logic into a separate function).

and there is basically no risk if the completion is not working fully.

Yes. I don’t think missing tests are a blocker.

Transports that reference a file or directory are completed. Signed-off-by: Yedaya Katsman <yedaya.ka@gmail.com>

yedayak force-pushed the transport-completions branch from 5e6d14c to 627aced Compare February 17, 2025 21:03

yedayak mentioned this pull request Feb 20, 2025

Allow setting prefix to remove before completing directories? spf13/cobra#2242

Open

yedayak force-pushed the transport-completions branch from 627aced to 580ed9f Compare February 21, 2025 15:17

yedayak marked this pull request as ready for review February 21, 2025 15:18

Don't complete a space after '<transport>:'

48a08d2

Signed-off-by: Yedaya Katsman <yedaya.ka@gmail.com>

yedayak force-pushed the transport-completions branch 2 times, most recently from 1539aa9 to 8f37068 Compare February 22, 2025 16:56

yedayak changed the title ~~WIP: Transport completions~~ Transport completions Feb 22, 2025

Luap99 reviewed Feb 27, 2025

View reviewed changes

yedayak force-pushed the transport-completions branch 2 times, most recently from 1f10c79 to 0dcebd9 Compare February 27, 2025 14:24

mtrmac reviewed Feb 27, 2025

View reviewed changes

Complete paths for some transports

d577111

Transports that reference a file or directory are completed. Signed-off-by: Yedaya Katsman <yedaya.ka@gmail.com>

yedayak force-pushed the transport-completions branch from 0dcebd9 to d577111 Compare March 2, 2025 11:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transport completions #2524

Transport completions #2524

yedayak commented Feb 17, 2025

Luap99 left a comment

Luap99 Feb 27, 2025

yedayak Feb 27, 2025

Luap99 Feb 27, 2025

Luap99 Feb 27, 2025

yedayak Feb 27, 2025

packit-as-a-service bot commented Feb 27, 2025

lsm5 commented Feb 27, 2025

Luap99 commented Feb 27, 2025

mtrmac left a comment

mtrmac Feb 27, 2025

yedayak Mar 2, 2025

mtrmac Feb 27, 2025

yedayak Mar 2, 2025

mtrmac Feb 27, 2025

yedayak Mar 2, 2025

mtrmac Feb 27, 2025

yedayak Mar 2, 2025

mtrmac commented Feb 27, 2025

Transport completions #2524

Are you sure you want to change the base?

Transport completions #2524

Conversation

yedayak commented Feb 17, 2025

Luap99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

packit-as-a-service bot commented Feb 27, 2025

lsm5 commented Feb 27, 2025

Luap99 commented Feb 27, 2025

mtrmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac commented Feb 27, 2025