refactor(logging): add a logging abstraction and move the job logger onto it (#1295)

Add `act/common/logging`: a printf-style `Logger` on a slog-shaped `Handler`, with a logrus-backed handler as the only backend, plus a `logtest` handler for tests. Move the job logger onto it: `common.Logger` returns `*logging.Logger`, the job log formatters and the `Reporter` hook become handlers, and composite step IDs travel in the context.

Logging is tightly coupled to logrus types (hooks, formatters, `Entry.Data`). Going through our own interface first lets the remaining call sites move over with unchanged behavior, and lets the backend be swapped for `log/slog` in one place afterwards. The global process logger and its masking formatter stay on logrus for now.

Assisted-by: Codet
Co-authored-by: bircni <[email protected]>
Reviewed-on: https://gitea.com/gitea/runner/pulls/1295
Reviewed-by: bircni <[email protected]>

act/container/docker_logger.go

@@ -12,7 +12,7 @@ import (
 	"errors"
 	"io"
 
-	"github.com/sirupsen/logrus"
+	"gitea.com/gitea/runner/act/common/logging"
 )
 
 type dockerMessage struct {
@@ -26,7 +26,7 @@ type dockerMessage struct {
 	Progress string `json:"progress"`
 }
 
-func logDockerResponse(logger logrus.FieldLogger, dockerResponse io.ReadCloser, isError bool) error {
+func logDockerResponse(logger *logging.Logger, dockerResponse io.ReadCloser, isError bool) error {
 	if dockerResponse == nil {
 		return nil
 	}
@@ -77,7 +77,7 @@ func logDockerResponse(logger logrus.FieldLogger, dockerResponse io.ReadCloser,
 	return nil
 }
 
-func writeLog(logger logrus.FieldLogger, isError bool, format string, args ...any) {
+func writeLog(logger *logging.Logger, isError bool, format string, args ...any) {
 	if isError {
 		logger.Errorf(format, args...)
 	} else {

act/container/docker_pull_test.go

@@ -10,15 +10,16 @@ import (
 	"strings"
 	"testing"
 
+	"gitea.com/gitea/runner/act/common/logging"
+	"gitea.com/gitea/runner/act/common/logging/logtest"
+
 	"github.com/docker/cli/cli/config"
-	log "github.com/sirupsen/logrus"
-	"github.com/sirupsen/logrus/hooks/test"
 	assert "github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
 func init() {
-	log.SetLevel(log.DebugLevel)
+	logging.SetLevel(logging.LevelDebug)
 }
 
 func TestCleanImage(t *testing.T) {
@@ -79,7 +80,7 @@ func TestIsPinnedImage(t *testing.T) {
 // The pull path reports a failure the daemon sent mid-stream, so it must carry the reason
 // whichever of the two shapes the daemon used.
 func TestLogDockerResponseError(t *testing.T) {
-	logger, _ := test.NewNullLogger()
+	logger := logging.New(logtest.New())
 	streamErr := func(line string) error {
 		return logDockerResponse(logger, io.NopCloser(strings.NewReader(line)), false)
 	}

act/container/docker_run.go

@@ -25,6 +25,7 @@ import (
 	"time"
 
 	"gitea.com/gitea/runner/act/common"
+	"gitea.com/gitea/runner/act/common/logging"
 
 	"dario.cat/mergo"
 	"github.com/bmatcuk/doublestar/v4"
@@ -40,7 +41,6 @@ import (
 	"github.com/moby/moby/client"
 	"github.com/moby/moby/client/pkg/versions"
 	specs "github.com/opencontainers/image-spec/specs-go/v1"
-	"github.com/sirupsen/logrus"
 )
 
 // drainGracePeriod bounds how long we wait for an output-copy goroutine to
@@ -1046,7 +1046,7 @@ func (cr *containerReference) wait() common.Executor {
 // sanitizeOptionsHostConfig takes back everything a workflow could escape the container with,
 // setting each field to trusted, which is what the runner's own options parse to on their own.
 // Only for unprivileged mode, since privileged mode grants host access anyway.
-func sanitizeOptionsHostConfig(logger logrus.FieldLogger, hostConfig, trusted *container.HostConfig) {
+func sanitizeOptionsHostConfig(logger *logging.Logger, hostConfig, trusted *container.HostConfig) {
 	resetOption(logger, "--pid", &hostConfig.PidMode, trusted.PidMode)
 	resetOption(logger, "--ipc", &hostConfig.IpcMode, trusted.IpcMode)
 	resetOption(logger, "--uts", &hostConfig.UTSMode, trusted.UTSMode)
@@ -1085,7 +1085,7 @@ func sanitizeOptionsHostConfig(logger logrus.FieldLogger, hostConfig, trusted *c
 
 // resetOption puts a field back to the runner's own value. It compares the values rather than
 // the flags, so a field that more than one option feeds cannot slip through.
-func resetOption[T any](logger logrus.FieldLogger, option string, field *T, trusted T) {
+func resetOption[T any](logger *logging.Logger, option string, field *T, trusted T) {
 	if reflect.DeepEqual(*field, trusted) {
 		return
 	}

act/container/docker_run_test.go

@@ -19,6 +19,8 @@ import (
 	"testing"
 
 	"gitea.com/gitea/runner/act/common"
+	"gitea.com/gitea/runner/act/common/logging"
+	"gitea.com/gitea/runner/act/common/logging/logtest"
 
 	cerrdefs "github.com/containerd/errdefs"
 	"github.com/moby/moby/api/pkg/stdcopy"
@@ -26,7 +28,6 @@ import (
 	"github.com/moby/moby/api/types/mount"
 	"github.com/moby/moby/api/types/network"
 	mobyclient "github.com/moby/moby/client"
-	"github.com/sirupsen/logrus/hooks/test"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
@@ -353,7 +354,8 @@ func TestRemoveIgnoresAutoRemoveRace(t *testing.T) {
 		{name: "real failure", err: removeFailure, wantErr: removeFailure},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
-			logger, hook := test.NewNullLogger()
+			hook := logtest.New()
+			logger := logging.New(hook)
 			ctx := common.WithLogger(context.Background(), logger)
 			client := &mockDockerClient{}
 			client.On("ContainerKill", ctx, "abc", killOpts).Return(mobyclient.ContainerKillResult{}, nil)
@@ -377,7 +379,7 @@ func TestRemoveIgnoresAutoRemoveRace(t *testing.T) {
 			} else {
 				assert.Empty(t, cr.id)
 			}
-			assert.Empty(t, hook.AllEntries())
+			assert.Empty(t, hook.AllRecords())
 			client.AssertExpectations(t)
 		})
 	}
@@ -614,7 +616,8 @@ func TestCheckVolumes(t *testing.T) {
 	}
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
-			logger, hook := test.NewNullLogger()
+			hook := logtest.New()
+			logger := logging.New(hook)
 			ctx := common.WithLogger(context.Background(), logger)
 			cr := &containerReference{
 				input: &NewContainerInput{
@@ -624,7 +627,7 @@ func TestCheckVolumes(t *testing.T) {
 			_, hostConf := cr.sanitizeConfig(ctx, &container.Config{}, &container.HostConfig{Binds: tc.binds, Mounts: tc.mounts})
 			assert.Equal(t, tc.expectedBinds, hostConf.Binds)
 			assert.ElementsMatch(t, tc.expectedMounts, hostConf.Mounts)
-			assert.Len(t, hook.AllEntries(), len(tc.binds)+len(tc.mounts)-len(tc.expectedBinds)-len(tc.expectedMounts)) // every drop is warned about
+			assert.Len(t, hook.AllRecords(), len(tc.binds)+len(tc.mounts)-len(tc.expectedBinds)-len(tc.expectedMounts)) // every drop is warned about
 		})
 	}
 }
@@ -652,7 +655,7 @@ func TestMergeContainerConfigsKeepsTheRunnersFilesAndEnvToItself(t *testing.T) {
 	t.Setenv("RUNNER_SECRET", "s3cr3t")
 
 	for _, option := range []string{"--env-file " + hostFile, "--label-file " + hostFile} {
-		logger, _ := test.NewNullLogger()
+		logger := logging.New(logtest.New())
 		cr := &containerReference{input: &NewContainerInput{NetworkMode: "bridge", WorkflowOptions: option}}
 
 		_, _, err := cr.mergeContainerConfigs(common.WithLogger(context.Background(), logger), &container.Config{}, &container.HostConfig{})
@@ -665,7 +668,7 @@ func TestMergeContainerConfigsKeepsTheRunnersFilesAndEnvToItself(t *testing.T) {
 	}
 
 	// a bare name is no longer resolved from the runner's environment, for either source
-	logger, _ := test.NewNullLogger()
+	logger := logging.New(logtest.New())
 	cr := &containerReference{input: &NewContainerInput{
 		NetworkMode:     "bridge",
 		RunnerOptions:   "--env RUNNER_SECRET",
@@ -678,7 +681,7 @@ func TestMergeContainerConfigsKeepsTheRunnersFilesAndEnvToItself(t *testing.T) {
 }
 
 func TestSanitizeOptionsHostConfig(t *testing.T) {
-	logger, _ := test.NewNullLogger()
+	logger := logging.New(logtest.New())
 
 	// every field the sanitizer resets, so a reset dropped in a refactor fails here
 	hostConfig := &container.HostConfig{
@@ -711,9 +714,10 @@ func TestSanitizeOptionsHostConfig(t *testing.T) {
 }
 
 // mergeOptions merges both option sources into a bare container, returning the result and its log.
-func mergeOptions(t *testing.T, runnerOptions, workflowOptions string, privileged bool) (*container.HostConfig, *test.Hook) {
+func mergeOptions(t *testing.T, runnerOptions, workflowOptions string, privileged bool) (*container.HostConfig, *logtest.Handler) {
 	t.Helper()
-	logger, hook := test.NewNullLogger()
+	hook := logtest.New()
+	logger := logging.New(hook)
 	cr := &containerReference{input: &NewContainerInput{
 		RunnerOptions:   runnerOptions,
 		WorkflowOptions: workflowOptions,
@@ -765,7 +769,7 @@ func TestMergeContainerConfigsStripsDangerousOptionsWhenUnprivileged(t *testing.
 }
 
 func TestCheckVolumesRejectsEscapingHostPaths(t *testing.T) {
-	logger, _ := test.NewNullLogger()
+	logger := logging.New(logtest.New())
 	ctx := common.WithLogger(context.Background(), logger)
 
 	base := t.TempDir()
@@ -806,14 +810,15 @@ func TestSanitizeConfigKeepsTmpfsMounts(t *testing.T) {
 	require.Len(t, hostConfig.Mounts, 1)
 
 	for _, validVolumes := range [][]string{nil, {"data"}, {"**:ro"}} {
-		logger, hook := test.NewNullLogger()
+		hook := logtest.New()
+		logger := logging.New(hook)
 		_, sanitized := (&containerReference{input: &NewContainerInput{ValidVolumes: validVolumes}}).sanitizeConfig(common.WithLogger(context.Background(), logger), &container.Config{}, &container.HostConfig{Mounts: []mount.Mount{
 			hostConfig.Mounts[0],
 			{Type: mount.TypeVolume, Target: "/anonymous"},
 			{Type: mount.TypeTmpfs, Source: "/etc", Target: "/etc-copy"},
 		}})
 		assert.Equal(t, hostConfig.Mounts, sanitized.Mounts)
-		assert.Len(t, hook.AllEntries(), 2)
+		assert.Len(t, hook.AllRecords(), 2)
 	}
 }
 
@@ -896,7 +901,7 @@ func TestContainerInfoFromInspect(t *testing.T) {
 }
 
 func TestMergeContainerConfigsVolumesReplaceRunnerMounts(t *testing.T) {
-	logger, _ := test.NewNullLogger()
+	logger := logging.New(logtest.New())
 	ctx := common.WithLogger(context.Background(), logger)
 	cr := &containerReference{
 		input: &NewContainerInput{
@@ -919,7 +924,7 @@ func TestMergeContainerConfigsVolumesReplaceRunnerMounts(t *testing.T) {
 func TestMergeContainerConfigsWarnsOnlyAboutOptionsThatWereGiven(t *testing.T) {
 	warnings := func(runnerOptions, workflowOptions string) int {
 		_, hook := mergeOptions(t, runnerOptions, workflowOptions, false)
-		return len(hook.AllEntries())
+		return len(hook.AllRecords())
 	}
 
 	assert.Zero(t, warnings("--volume /host/tools:/opt/hostedtoolcache", ""))
@@ -931,7 +936,8 @@ func TestMergeContainerConfigsWarnsOnlyAboutOptionsThatWereGiven(t *testing.T) {
 func TestDockerCreateKeepsInputEntrypoint(t *testing.T) {
 	for _, entrypoint := range [][]string{nil, {"sleep", "3600"}} {
 		for _, optionEntrypoint := range []string{"/sbin/dumb-init", ""} {
-			logger, hook := test.NewNullLogger()
+			hook := logtest.New()
+			logger := logging.New(hook)
 			cr := &containerReference{
 				input: &NewContainerInput{
 					NetworkMode:     "bridge",
@@ -952,13 +958,13 @@ func TestDockerCreateKeepsInputEntrypoint(t *testing.T) {
 			}
 			require.NoError(t, cr.create(nil, nil)(common.WithLogger(t.Context(), logger)))
 			assert.Equal(t, "created", cr.id)
-			assert.Equal(t, len(entrypoint) > 0, len(hook.AllEntries()) == 1)
+			assert.Equal(t, len(entrypoint) > 0, len(hook.AllRecords()) == 1)
 		}
 	}
 }
 
 func TestMergeContainerConfigsKeepsNetworkAliasesFromOptions(t *testing.T) {
-	logger, _ := test.NewNullLogger()
+	logger := logging.New(logtest.New())
 	cr := &containerReference{input: &NewContainerInput{
 		NetworkMode: "job-network", NetworkAliases: []string{"redis"}, WorkflowOptions: "--network-alias redis-primary",
 	}}

act/container/host_environment_test.go

@@ -18,8 +18,8 @@ import (
 	"time"
 
 	"gitea.com/gitea/runner/act/common"
+	"gitea.com/gitea/runner/act/common/logging"
 
-	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -170,8 +170,7 @@ func TestHostEnvironmentAllocatePTY(t *testing.T) {
 }
 
 func TestHostEnvironmentRemovePreservesWorkdirByDefault(t *testing.T) {
-	logger := logrus.New()
-	ctx := common.WithLogger(context.Background(), logrus.NewEntry(logger))
+	ctx := common.WithLogger(context.Background(), logging.Default())
 	base := t.TempDir()
 	miscRoot := filepath.Join(base, "misc")
 	path := filepath.Join(miscRoot, "hostexecutor")
@@ -193,8 +192,7 @@ func TestHostEnvironmentRemovePreservesWorkdirByDefault(t *testing.T) {
 }
 
 func TestHostEnvironmentRemoveCleansWorkdirWhenOwned(t *testing.T) {
-	logger := logrus.New()
-	ctx := common.WithLogger(context.Background(), logrus.NewEntry(logger))
+	ctx := common.WithLogger(context.Background(), logging.Default())
 	base := t.TempDir()
 	miscRoot := filepath.Join(base, "misc")
 	path := filepath.Join(miscRoot, "hostexecutor")
@@ -252,8 +250,7 @@ func TestHostEnvironmentRemoveDoesNotHangOnStuckCleanUp(t *testing.T) {
 	hostCleanupTimeout = 100 * time.Millisecond
 	t.Cleanup(func() { hostCleanupTimeout = orig })
 
-	logger := logrus.New()
-	ctx := common.WithLogger(context.Background(), logrus.NewEntry(logger))
+	ctx := common.WithLogger(context.Background(), logging.Default())
 	base := t.TempDir()
 	path := filepath.Join(base, "misc", "hostexecutor")
 	require.NoError(t, os.MkdirAll(path, 0o700))
@@ -306,8 +303,7 @@ func TestHostEnvironmentRemoveDoesNotHangOnStuckPathRemoval(t *testing.T) {
 		removeAll = origRemoveAll
 	})
 
-	logger := logrus.New()
-	ctx := common.WithLogger(context.Background(), logrus.NewEntry(logger))
+	ctx := common.WithLogger(context.Background(), logging.Default())
 	base := t.TempDir()
 	path := filepath.Join(base, "misc", "hostexecutor")
 	require.NoError(t, os.MkdirAll(path, 0o700))