Merge branch '3915-default-cache-volume-when-registering-windows-docker-executor' into 'master'

Fix default cache volume docker-windows register

Closes #3915

See merge request !1391

commands/register.go

@@ -2,6 +2,7 @@ package commands
 
 import (
 	"bufio"
+	"fmt"
 	"os"
 	"os/signal"
 	"strings"
@@ -41,6 +42,10 @@ const (
 	RefProtected AccessLevel = "ref_protected"
 )
 
+const (
+	defaultDockerWindowCacheDir = "c:\\cache"
+)
+
 func (s *RegisterCommand) askOnce(prompt string, result *string, allowEmpty bool) bool {
 	println(prompt)
 	if *result != "" {
@@ -110,10 +115,7 @@ func (s *RegisterCommand) askExecutor() {
 }
 
 func (s *RegisterCommand) askDocker() {
-	if s.Docker == nil {
-		s.Docker = &common.DockerConfig{}
-	}
-	s.Docker.Image = s.ask("docker-image", "Please enter the default Docker image (e.g. ruby:2.1):")
+	s.askBasicDocker("ruby:2.6")
 
 	for _, volume := range s.Docker.Volumes {
 		parts := strings.Split(volume, ":")
@@ -124,6 +126,27 @@ func (s *RegisterCommand) askDocker() {
 	s.Docker.Volumes = append(s.Docker.Volumes, "/cache")
 }
 
+func (s *RegisterCommand) askDockerWindows() {
+	s.askBasicDocker("mcr.microsoft.com/windows/servercore:1809")
+
+	for _, volume := range s.Docker.Volumes {
+		// This does not cover all the possibilities since we don't have access
+		// to volume parsing package since it's internal.
+		if strings.Contains(volume, defaultDockerWindowCacheDir) {
+			return
+		}
+	}
+	s.Docker.Volumes = append(s.Docker.Volumes, defaultDockerWindowCacheDir)
+}
+
+func (s *RegisterCommand) askBasicDocker(exampleHelperImage string) {
+	if s.Docker == nil {
+		s.Docker = &common.DockerConfig{}
+	}
+
+	s.Docker.Image = s.ask("docker-image", fmt.Sprintf("Please enter the default Docker image (e.g. %s):", exampleHelperImage))
+}
+
 func (s *RegisterCommand) askParallels() {
 	s.Parallels.BaseName = s.ask("parallels-base-name", "Please enter the Parallels VM (e.g. my-vm):")
 }
@@ -223,7 +246,7 @@ func (s *RegisterCommand) askExecutorOptions() {
 		},
 		"docker-windows": func() {
 			s.Docker = docker
-			s.askDocker()
+			s.askDockerWindows()
 		},
 		"docker-ssh": func() {
 			s.Docker = docker

commands/register_windows_test.go

+package commands
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"gitlab.com/gitlab-org/gitlab-runner/common"
+)
+
+func TestRegisterDefaultWindowsDockerCacheVolume(t *testing.T) {
+	testCases := map[string]struct {
+		userDefinedVolumes []string
+		expectedVolumes    []string
+	}{
+		"user did not define anything": {
+			userDefinedVolumes: []string{},
+			expectedVolumes:    []string{defaultDockerWindowCacheDir},
+		},
+		"user defined an extra volume": {
+			userDefinedVolumes: []string{"c:\\Users\\SomeUser\\config.json:c:\\config.json"},
+			expectedVolumes:    []string{defaultDockerWindowCacheDir, "c:\\Users\\SomeUser\\config.json:c:\\config.json"},
+		},
+		"user defined volume binding to default cache dir": {
+			userDefinedVolumes: []string{fmt.Sprintf("c:\\Users\\SomeUser\\cache:%s", defaultDockerWindowCacheDir)},
+			expectedVolumes:    []string{fmt.Sprintf("c:\\Users\\SomeUser\\cache:%s", defaultDockerWindowCacheDir)},
+		},
+		"user defined cache as source leads to incorrect parsing of volume and never adds cache volume": {
+			userDefinedVolumes: []string{"c:\\cache:c:\\User\\ContainerAdministrator\\cache"},
+			expectedVolumes:    []string{"c:\\cache:c:\\User\\ContainerAdministrator\\cache"},
+		},
+	}
+
+	for name, testCase := range testCases {
+		t.Run(name, func(t *testing.T) {
+			s := setupDockerRegisterCommand(&common.DockerConfig{
+				Volumes: testCase.userDefinedVolumes,
+			})
+
+			s.askDockerWindows()
+			assert.ElementsMatch(t, testCase.expectedVolumes, s.Docker.Volumes)
+		})
+	}
+}

docs/executors/docker.md

@@ -71,11 +71,11 @@ be used:
 
 ### Configuring a Windows Docker executor
 
-An incorrect Windows path for the
-[`volumes`](../configuration/advanced-configuration.md#the-runnersdocker-section)
-key is defined in `config.toml` file. User must manually edit this key
-until [#3915](https://gitlab.com/gitlab-org/gitlab-runner/issues/3915)
-is resolved.
+NOTE: **Note:**
+There is a known issue when a new Runner is registered with `c:\\cache`
+as a source directory when passing the `--docker-volumes` or
+`DOCKER_VOLUMES` envrionment variable. For more details check
+[#4312](https://gitlab.com/gitlab-org/gitlab-runner/issues/4312)
 
 Below is an example of what the configuration for a simple Docker
 executor running Windows