Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions cli-plugins/manager/hooks.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,10 @@ func invokeAndCollectHooks(ctx context.Context, cfg *configfile.ConfigFile, root
return nil
}

pluginDirs, err := getPluginDirs(cfg)
if err != nil {
return nil
}
pluginDirs := getPluginDirs(cfg)
nextSteps := make([]string, 0, len(pluginsCfg))
for pluginName, cfg := range pluginsCfg {
match, ok := pluginMatch(cfg, subCmdStr)
for pluginName, pluginCfg := range pluginsCfg {
match, ok := pluginMatch(pluginCfg, subCmdStr)
if !ok {
continue
}
Expand Down
36 changes: 15 additions & 21 deletions cli-plugins/manager/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,20 +61,16 @@ func IsNotFound(err error) bool {
// 3. Platform-specific defaultSystemPluginDirs.
//
// [ConfigFile.CLIPluginsExtraDirs]: https://pkg.go.dev/github.com/docker/cli@v26.1.4+incompatible/cli/config/configfile#ConfigFile.CLIPluginsExtraDirs
func getPluginDirs(cfg *configfile.ConfigFile) ([]string, error) {
func getPluginDirs(cfg *configfile.ConfigFile) []string {
var pluginDirs []string

if cfg != nil {
pluginDirs = append(pluginDirs, cfg.CLIPluginsExtraDirs...)
}
pluginDir, err := config.Path("cli-plugins")
if err != nil {
return nil, err
}

pluginDir := filepath.Join(config.Dir(), "cli-plugins")
pluginDirs = append(pluginDirs, pluginDir)
pluginDirs = append(pluginDirs, defaultSystemPluginDirs...)
return pluginDirs, nil
return pluginDirs
}

func addPluginCandidatesFromDir(res map[string][]string, d string) {
Expand Down Expand Up @@ -116,10 +112,7 @@ func listPluginCandidates(dirs []string) map[string][]string {

// GetPlugin returns a plugin on the system by its name
func GetPlugin(name string, dockerCLI config.Provider, rootcmd *cobra.Command) (*Plugin, error) {
pluginDirs, err := getPluginDirs(dockerCLI.ConfigFile())
if err != nil {
return nil, err
}
pluginDirs := getPluginDirs(dockerCLI.ConfigFile())
return getPlugin(name, pluginDirs, rootcmd)
}

Expand All @@ -145,16 +138,20 @@ func getPlugin(name string, pluginDirs []string, rootcmd *cobra.Command) (*Plugi

// ListPlugins produces a list of the plugins available on the system
func ListPlugins(dockerCli config.Provider, rootcmd *cobra.Command) ([]Plugin, error) {
pluginDirs, err := getPluginDirs(dockerCli.ConfigFile())
if err != nil {
return nil, err
}

pluginDirs := getPluginDirs(dockerCli.ConfigFile())
candidates := listPluginCandidates(pluginDirs)
if len(candidates) == 0 {
return nil, nil
}

var plugins []Plugin
var mu sync.Mutex
eg, _ := errgroup.WithContext(context.TODO())
ctx := rootcmd.Context()
if ctx == nil {
// Fallback, mostly for tests that pass a bare cobra.command
ctx = context.Background()
}
eg, _ := errgroup.WithContext(ctx)
Comment on lines +150 to +154
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

maybe we should instead force the tests to set a context?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, I originally updated the test, but given that this function is exported, I could not 100% exclude the possibility for other code to call this with a command that has not (yet?) has a context set.

Without a context, this produces a panic at runtime, which was a bit risky, so ultimately I chose for making this "best effort", and keep the test as-is to verify that things work even if no context is present on the command.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah i see, alright then let's get this one merged.

cmds := rootcmd.Commands()
for _, paths := range candidates {
func(paths []string) {
Expand Down Expand Up @@ -202,10 +199,7 @@ func PluginRunCommand(dockerCli config.Provider, name string, rootcmd *cobra.Com
return nil, errPluginNotFound(name)
}
exename := addExeSuffix(metadata.NamePrefix + name)
pluginDirs, err := getPluginDirs(dockerCli.ConfigFile())
if err != nil {
return nil, err
}
pluginDirs := getPluginDirs(dockerCli.ConfigFile())

for _, d := range pluginDirs {
path := filepath.Join(d, exename)
Expand Down
17 changes: 10 additions & 7 deletions cli-plugins/manager/manager_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package manager

import (
"path/filepath"
"strings"
"testing"

Expand Down Expand Up @@ -81,6 +82,12 @@ func TestListPluginCandidates(t *testing.T) {
assert.DeepEqual(t, candidates, exp)
}

func TestListPluginCandidatesEmpty(t *testing.T) {
tmpDir := t.TempDir()
candidates := listPluginCandidates([]string{tmpDir, filepath.Join(tmpDir, "no-such-dir")})
assert.Assert(t, len(candidates) == 0)
}

// Regression test for https://github.com/docker/cli/issues/5643.
// Check that inaccessible directories that come before accessible ones are ignored
// and do not prevent the latter from being processed.
Expand Down Expand Up @@ -166,14 +173,11 @@ func TestErrPluginNotFound(t *testing.T) {
func TestGetPluginDirs(t *testing.T) {
cli := test.NewFakeCli(nil)

pluginDir, err := config.Path("cli-plugins")
assert.NilError(t, err)
pluginDir := filepath.Join(config.Dir(), "cli-plugins")
expected := append([]string{pluginDir}, defaultSystemPluginDirs...)

var pluginDirs []string
pluginDirs, err = getPluginDirs(cli.ConfigFile())
pluginDirs := getPluginDirs(cli.ConfigFile())
assert.Equal(t, strings.Join(expected, ":"), strings.Join(pluginDirs, ":"))
assert.NilError(t, err)

extras := []string{
"foo", "bar", "baz",
Expand All @@ -182,7 +186,6 @@ func TestGetPluginDirs(t *testing.T) {
cli.SetConfigFile(&configfile.ConfigFile{
CLIPluginsExtraDirs: extras,
})
pluginDirs, err = getPluginDirs(cli.ConfigFile())
pluginDirs = getPluginDirs(cli.ConfigFile())
assert.DeepEqual(t, expected, pluginDirs)
assert.NilError(t, err)
}