Refactor docker backend and add more test coverage (#2700)

collection of some smal nit's and additions of tests

pipeline/backend/common/script_test.go

@@ -1,3 +1,17 @@
+// Copyright 2024 Woodpecker Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
 package common
 
 import (

pipeline/backend/docker/convert.go

@@ -41,6 +41,7 @@ func (e *docker) toConfig(step *types.Step) *container.Config {
 		WorkingDir:   step.WorkingDir,
 		AttachStdout: true,
 		AttachStderr: true,
+		Volumes:      toVol(step.Volumes),
 	}
 	configEnv := make(map[string]string)
 	maps.Copy(configEnv, step.Environment)
@@ -59,9 +60,6 @@ func (e *docker) toConfig(step *types.Step) *container.Config {
 	if len(configEnv) != 0 {
 		config.Env = toEnv(configEnv)
 	}
-	if len(step.Volumes) != 0 {
-		config.Volumes = toVol(step.Volumes)
-	}
 	return config
 }
 
@@ -127,7 +125,10 @@ func toHostConfig(step *types.Step) *container.HostConfig {
 // helper function that converts a slice of volume paths to a set of
 // unique volume names.
 func toVol(paths []string) map[string]struct{} {
-	set := map[string]struct{}{}
+	if len(paths) == 0 {
+		return nil
+	}
+	set := make(map[string]struct{})
 	for _, path := range paths {
 		parts, err := splitVolumeParts(path)
 		if err != nil {
@@ -146,8 +147,10 @@ func toVol(paths []string) map[string]struct{} {
 func toEnv(env map[string]string) []string {
 	var envs []string
 	for k, v := range env {
+		if k != "" {
 			envs = append(envs, k+"="+v)
 		}
+	}
 	return envs
 }
 

pipeline/backend/docker/convert_test.go

@@ -91,6 +91,80 @@ func TestSplitVolumeParts(t *testing.T) {
 	}
 }
 
+// dummy vars to test against.
+var (
+	testCmdStep = &backend.Step{
+		Name:        "hello",
+		UUID:        "f51821af-4cb8-435e-a3c2-3a684185d828",
+		Type:        backend.StepTypeCommands,
+		Commands:    []string{"echo \"hello world\"", "ls"},
+		Image:       "alpine",
+		Environment: map[string]string{"SHELL": "/bin/zsh"},
+	}
+
+	testPluginStep = &backend.Step{
+		Name:        "lint",
+		UUID:        "d841ee40-e66e-4275-bb3f-55bf89744b21",
+		Type:        backend.StepTypePlugin,
+		Image:       "mstruebing/editorconfig-checker",
+		Environment: make(map[string]string),
+	}
+
+	testEngine = &docker{
+		info: types.Info{
+			Architecture:    "x86_64",
+			OSType:          "linux",
+			DefaultRuntime:  "runc",
+			DockerRootDir:   "/var/lib/docker",
+			OperatingSystem: "Archlinux",
+			Name:            "SOME_HOSTNAME",
+		},
+	}
+)
+
+func TestToContainerName(t *testing.T) {
+	assert.EqualValues(t, "wp_f51821af-4cb8-435e-a3c2-3a684185d828", toContainerName(testCmdStep))
+	assert.EqualValues(t, "wp_d841ee40-e66e-4275-bb3f-55bf89744b21", toContainerName(testPluginStep))
+}
+
+func TestStepToConfig(t *testing.T) {
+	// StepTypeCommands
+	conf := testEngine.toConfig(testCmdStep)
+	if assert.NotNil(t, conf) {
+		assert.EqualValues(t, []string{"/bin/sh", "-c", "echo $CI_SCRIPT | base64 -d | /bin/sh -e"}, conf.Entrypoint)
+		assert.Nil(t, conf.Cmd)
+		assert.EqualValues(t, testCmdStep.UUID, conf.Labels["wp_uuid"])
+	}
+
+	// StepTypePlugin
+	conf = testEngine.toConfig(testPluginStep)
+	if assert.NotNil(t, conf) {
+		assert.Nil(t, conf.Cmd)
+		assert.EqualValues(t, testPluginStep.UUID, conf.Labels["wp_uuid"])
+	}
+}
+
+func TestToEnv(t *testing.T) {
+	assert.Nil(t, toEnv(nil))
+	assert.EqualValues(t, []string{"A=B"}, toEnv(map[string]string{"A": "B"}))
+	assert.ElementsMatch(t, []string{"A=B=C", "T=T"}, toEnv(map[string]string{"A": "B=C", "": "Z", "T": "T"}))
+}
+
+func TestToVol(t *testing.T) {
+	assert.Nil(t, toVol(nil))
+	assert.EqualValues(t, map[string]struct{}{"/test": {}}, toVol([]string{"test:/test"}))
+}
+
+func TestEncodeAuthToBase64(t *testing.T) {
+	res, err := encodeAuthToBase64(backend.Auth{})
+	assert.NoError(t, err)
+	assert.EqualValues(t, "e30=", res)
+
+	res, err = encodeAuthToBase64(backend.Auth{Username: "user", Password: "pwd"})
+	assert.NoError(t, err)
+	assert.EqualValues(t, "eyJ1c2VybmFtZSI6InVzZXIiLCJwYXNzd29yZCI6InB3ZCJ9", res)
+}
+
 func TestToConfigSmall(t *testing.T) {
 	engine := docker{info: types.Info{OSType: "linux/riscv64"}}
 

pipeline/backend/docker/docker.go

@@ -257,7 +257,7 @@ func (e *docker) StartStep(ctx context.Context, step *backend.Step, taskUUID str
 		}
 	}
 
-	return e.client.ContainerStart(ctx, containerName, startOpts)
+	return e.client.ContainerStart(ctx, containerName, types.ContainerStartOptions{})
 }
 
 func (e *docker) WaitStep(ctx context.Context, step *backend.Step, taskUUID string) (*backend.State, error) {
@@ -286,7 +286,13 @@ func (e *docker) WaitStep(ctx context.Context, step *backend.Step, taskUUID stri
 func (e *docker) TailStep(ctx context.Context, step *backend.Step, taskUUID string) (io.ReadCloser, error) {
 	log.Trace().Str("taskUUID", taskUUID).Msgf("tail logs of step %s", step.Name)
 
-	logs, err := e.client.ContainerLogs(ctx, toContainerName(step), logsOpts)
+	logs, err := e.client.ContainerLogs(ctx, toContainerName(step), types.ContainerLogsOptions{
+		Follow:     true,
+		ShowStdout: true,
+		ShowStderr: true,
+		Details:    false,
+		Timestamps: false,
+	})
 	if err != nil {
 		return nil, err
 	}
@@ -344,23 +350,11 @@ func (e *docker) DestroyWorkflow(ctx context.Context, conf *backend.Config, task
 	return nil
 }
 
-var (
+var removeOpts = types.ContainerRemoveOptions{
-	startOpts = types.ContainerStartOptions{}
-
-	removeOpts = types.ContainerRemoveOptions{
 	RemoveVolumes: true,
 	RemoveLinks:   false,
 	Force:         false,
-	}
+}
-
-	logsOpts = types.ContainerLogsOptions{
-		Follow:     true,
-		ShowStdout: true,
-		ShowStderr: true,
-		Details:    false,
-		Timestamps: false,
-	}
-)
 
 func isErrContainerNotFoundOrNotRunning(err error) bool {
 	// Error response from daemon: Cannot kill container: ...: No such container: ...

pipeline/backend/kubernetes/kubernetes.go

@@ -395,7 +395,6 @@ func (e *kube) DestroyStep(ctx context.Context, step *types.Step, taskUUID strin
 func (e *kube) DestroyWorkflow(ctx context.Context, conf *types.Config, taskUUID string) error {
 	log.Trace().Str("taskUUID", taskUUID).Msg("deleting Kubernetes primitives")
 
-	// Use noContext because the ctx sent to this function will be canceled/done in case of error or canceled by user.
 	for _, stage := range conf.Stages {
 		for _, step := range stage.Steps {
 			err := stopPod(ctx, e, step, defaultDeleteOptions)