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 <[email protected]>
rancher-sandbox · Sep 11, 2024 · 6301957 · 6301957
1 parent 6b38b5a
commit 6301957
Show file tree

Hide file tree

Showing 6 changed files with 19 additions and 21 deletions.
diff --git 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
@@ -15,17 +15,17 @@ 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<void> {
+async function createTestSymlinks(integrationDirectory: string, dockerCLIPluginDest: string): Promise<void> {
   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');
 
   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
@@ -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
@@ -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
@@ -29,7 +29,7 @@ import (
 )
 
 const (
-	dirsKey = "cliPluginsExtraDirs"
+	pluginDirsKey = "cliPluginsExtraDirs"
 )
 
 // SetupPluginDirConfig configures docker CLI to load plugins from the directory
@@ -54,25 +54,25 @@ 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 {
 			if index >= 0 {
 				// 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
@@ -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.