From 6301957867be104e1a60587a72dd6fb5cf222d98 Mon Sep 17 00:00:00 2001 From: Mark Yen Date: Wed, 11 Sep 2024 11:56:35 -0700 Subject: [PATCH] Address review comments - Exclude docker credential helpers when checking for docker CLI plugins in moby client - Rename various variables - Other misc fixes Signed-off-by: Mark Yen --- .../backend/containerClient/mobyClient.ts | 3 ++- .../__tests__/unixIntegrationManager.spec.ts | 6 +++--- .../integrations/integrationManager.ts | 3 --- .../cmd/wsl_integration_docker_plugin_linux.go | 4 ++-- .../pkg/integration/docker_plugin_linux.go | 14 +++++++------- .../pkg/integration/docker_plugin_linux_test.go | 10 +++++----- 6 files changed, 19 insertions(+), 21 deletions(-) diff --git a/pkg/rancher-desktop/backend/containerClient/mobyClient.ts b/pkg/rancher-desktop/backend/containerClient/mobyClient.ts index 2126c86efb6..713bd0e6248 100644 --- a/pkg/rancher-desktop/backend/containerClient/mobyClient.ts +++ b/pkg/rancher-desktop/backend/containerClient/mobyClient.ts @@ -471,7 +471,8 @@ export class MobyClient implements ContainerEngineClient { runClient(args: string[], stdio: 'stream', options?: runClientOptions): ReadableProcess; runClient(args: string[], stdio?: 'ignore' | 'pipe' | 'stream' | Log, options?: runClientOptions) { const executableName = options?.executable ?? this.executable; - const binType = executableName.startsWith('docker-') ? 'docker-cli-plugins' : 'bin'; + const isCLIPlugin = /^docker-(?!credential-)/.test(executableName); + const binType = isCLIPlugin ? 'docker-cli-plugins' : 'bin'; const binDir = path.join(paths.resources, process.platform, binType); const executable = path.resolve(binDir, executableName); const opts = _.merge({}, options ?? {}, { diff --git a/pkg/rancher-desktop/integrations/__tests__/unixIntegrationManager.spec.ts b/pkg/rancher-desktop/integrations/__tests__/unixIntegrationManager.spec.ts index e25c0654767..18e97de3789 100644 --- a/pkg/rancher-desktop/integrations/__tests__/unixIntegrationManager.spec.ts +++ b/pkg/rancher-desktop/integrations/__tests__/unixIntegrationManager.spec.ts @@ -15,9 +15,9 @@ let testDir: string; // Creates integration directory and docker CLI plugin directory with // relevant symlinks in them. Useful for testing removal parts // of UnixIntegrationManager. -async function createTestSymlinks(integrationDirectory: string, dockerCliPluginDirectory: string): Promise { +async function createTestSymlinks(integrationDirectory: string, dockerCLIPluginDest: string): Promise { await fs.promises.mkdir(integrationDirectory, { recursive: true, mode: 0o755 }); - await fs.promises.mkdir(dockerCliPluginDirectory, { recursive: true, mode: 0o755 }); + await fs.promises.mkdir(dockerCLIPluginDest, { recursive: true, mode: 0o755 }); const kubectlSrcPath = path.join(binDir, 'kubectl'); const kubectlDstPath = path.join(integrationDirectory, 'kubectl'); @@ -25,7 +25,7 @@ async function createTestSymlinks(integrationDirectory: string, dockerCliPluginD await fs.promises.symlink(kubectlSrcPath, kubectlDstPath); const composeSrcPath = path.join(dockerCLIPluginSource, 'docker-compose'); - const composeDstPath = path.join(dockerCliPluginDirectory, 'docker-compose'); + const composeDstPath = path.join(dockerCLIPluginDest, 'docker-compose'); await fs.promises.symlink(composeSrcPath, composeDstPath); } diff --git a/pkg/rancher-desktop/integrations/integrationManager.ts b/pkg/rancher-desktop/integrations/integrationManager.ts index eca9d5b880d..750eee7aef1 100644 --- a/pkg/rancher-desktop/integrations/integrationManager.ts +++ b/pkg/rancher-desktop/integrations/integrationManager.ts @@ -48,9 +48,6 @@ export function getIntegrationManager(): IntegrationManager { switch (platform) { case 'linux': - return new UnixIntegrationManager({ - binDir, integrationDir: paths.integration, dockerCLIPluginSource, dockerCLIPluginDest, - }); case 'darwin': return new UnixIntegrationManager({ binDir, integrationDir: paths.integration, dockerCLIPluginSource, dockerCLIPluginDest, diff --git a/src/go/wsl-helper/cmd/wsl_integration_docker_plugin_linux.go b/src/go/wsl-helper/cmd/wsl_integration_docker_plugin_linux.go index 907114ccf52..2dbdb0055bb 100644 --- a/src/go/wsl-helper/cmd/wsl_integration_docker_plugin_linux.go +++ b/src/go/wsl-helper/cmd/wsl_integration_docker_plugin_linux.go @@ -36,14 +36,14 @@ var wslIntegrationDockerPluginCmd = &cobra.Command{ cmd.SilenceUsage = true state := wslIntegrationDockerPluginViper.GetBool("state") - pluginPath := wslIntegrationDockerPluginViper.GetString("plugin-dir") + pluginDir := wslIntegrationDockerPluginViper.GetString("plugin-dir") binDir := wslIntegrationDockerPluginViper.GetString("bin-dir") homeDir, err := os.UserHomeDir() if err != nil { return fmt.Errorf("failed to locate home directory: %w", err) } - if err := integration.SetupPluginDirConfig(homeDir, pluginPath, state); err != nil { + if err := integration.SetupPluginDirConfig(homeDir, pluginDir, state); err != nil { return err } diff --git a/src/go/wsl-helper/pkg/integration/docker_plugin_linux.go b/src/go/wsl-helper/pkg/integration/docker_plugin_linux.go index 9a3fffa3523..fd9d04ad480 100644 --- a/src/go/wsl-helper/pkg/integration/docker_plugin_linux.go +++ b/src/go/wsl-helper/pkg/integration/docker_plugin_linux.go @@ -29,7 +29,7 @@ import ( ) const ( - dirsKey = "cliPluginsExtraDirs" + pluginDirsKey = "cliPluginsExtraDirs" ) // SetupPluginDirConfig configures docker CLI to load plugins from the directory @@ -54,17 +54,17 @@ func SetupPluginDirConfig(homeDir, pluginPath string, enabled bool) error { var dirs []string - if dirsRaw, ok := config[dirsKey]; ok { + if dirsRaw, ok := config[pluginDirsKey]; ok { if dirsAny, ok := dirsRaw.([]any); ok { for _, item := range dirsAny { if dir, ok := item.(string); ok { dirs = append(dirs, dir) } else { - return fmt.Errorf("failed to update docker CLI configuration: %q has non-string item %v", dirsKey, item) + return fmt.Errorf("failed to update docker CLI configuration: %q has non-string item %v", pluginDirsKey, item) } } } else { - return fmt.Errorf("failed to update docker CLI configuration: %q is not a string array", dirsKey) + return fmt.Errorf("failed to update docker CLI configuration: %q is not a string array", pluginDirsKey) } index := slices.Index(dirs, pluginPath) if enabled { @@ -72,7 +72,7 @@ func SetupPluginDirConfig(homeDir, pluginPath string, enabled bool) error { // Config file already contains the plugin path; nothing to do. return nil } - dirs = append(dirs, pluginPath) + dirs = append([]string{pluginPath}, dirs...) } else { if index < 0 { // Config does not contain the plugin path; nothing to do. @@ -89,9 +89,9 @@ func SetupPluginDirConfig(homeDir, pluginPath string, enabled bool) error { dirs = []string{pluginPath} } if len(dirs) > 0 { - config[dirsKey] = dirs + config[pluginDirsKey] = dirs } else { - delete(config, dirsKey) + delete(config, pluginDirsKey) } if configBytes, err = json.Marshal(config); err != nil { diff --git a/src/go/wsl-helper/pkg/integration/docker_plugin_linux_test.go b/src/go/wsl-helper/pkg/integration/docker_plugin_linux_test.go index 16f36656d66..9ffa4af9da7 100644 --- a/src/go/wsl-helper/pkg/integration/docker_plugin_linux_test.go +++ b/src/go/wsl-helper/pkg/integration/docker_plugin_linux_test.go @@ -79,7 +79,7 @@ func TestSetupPluginDirConfig(t *testing.T) { require.NoError(t, integration.SetupPluginDirConfig(homeDir, pluginPath, true)) - bytes, err := os.ReadFile(path.Join(homeDir, ".docker", "config.json")) + bytes, err := os.ReadFile(configPath) require.NoError(t, err, "error reading docker CLI config") require.NoError(t, json.Unmarshal(bytes, &config)) @@ -99,7 +99,7 @@ func TestSetupPluginDirConfig(t *testing.T) { require.NoError(t, integration.SetupPluginDirConfig(homeDir, pluginPath, false)) - bytes, err := os.ReadFile(path.Join(homeDir, ".docker", "config.json")) + bytes, err := os.ReadFile(configPath) require.NoError(t, err, "error reading docker CLI config") require.NoError(t, json.Unmarshal(bytes, &config)) @@ -117,7 +117,7 @@ func TestSetupPluginDirConfig(t *testing.T) { assert.Error(t, integration.SetupPluginDirConfig(homeDir, pluginPath, true)) - bytes, err := os.ReadFile(path.Join(homeDir, ".docker", "config.json")) + bytes, err := os.ReadFile(configPath) require.NoError(t, err, "error reading docker CLI config") assert.Equal(t, existingContents, bytes, "docker CLI config was changed") }) @@ -134,7 +134,7 @@ func TestSetupPluginDirConfig(t *testing.T) { require.Error(t, integration.SetupPluginDirConfig(homeDir, pluginPath, false)) - bytes, err := os.ReadFile(path.Join(homeDir, ".docker", "config.json")) + bytes, err := os.ReadFile(configPath) require.NoError(t, err, "error reading docker CLI config") // Since we should not have modified the file at all, the file should // still be byte-identical. @@ -154,7 +154,7 @@ func TestSetupPluginDirConfig(t *testing.T) { require.Error(t, integration.SetupPluginDirConfig(homeDir, pluginPath, false)) - bytes, err := os.ReadFile(path.Join(homeDir, ".docker", "config.json")) + bytes, err := os.ReadFile(configPath) require.NoError(t, err, "error reading docker CLI config") // Since we should not have modified the file at all, the file should // still be byte-identical.