From 8a8e01522d249399992a29cd4ecb9594cd69d416 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Thu, 14 Aug 2025 10:21:08 +0200 Subject: [PATCH] chore: refactor network name creation - networkName becomes ensureNetworkName and stores the results in the run context instead of returning it - the getNetworkName and getNetworkCreated accessors are used instead of local variables --- act/runner/job_executor.go | 8 +--- act/runner/run_context.go | 55 +++++++++++------------ act/runner/run_context_test.go | 82 ++++++++++++++++++++++++++++++++++ 3 files changed, 110 insertions(+), 35 deletions(-) diff --git a/act/runner/job_executor.go b/act/runner/job_executor.go index 848e56f8..844d2fed 100644 --- a/act/runner/job_executor.go +++ b/act/runner/job_executor.go @@ -130,12 +130,8 @@ func newJobExecutor(info jobInfo, sf stepFactory, rc *RunContext) common.Executo logger.Errorf("Error while stop job container %s: %v", rc.jobContainerName(), err) } - if !rc.IsHostEnv(ctx) && rc.Config.ContainerNetworkMode == "" { - // clean network in docker mode only - // if the value of `ContainerNetworkMode` is empty string, - // it means that the network to which containers are connecting is created by `act_runner`, - // so, we should remove the network at last. - networkName, _ := rc.networkName() + if !rc.IsHostEnv(ctx) && rc.getNetworkCreated(ctx) { + networkName := rc.getNetworkName(ctx) logger.Debugf("Cleaning up network %s for job %s", networkName, rc.jobContainerName()) if err := container.NewDockerNetworkRemoveExecutor(networkName)(ctx); err != nil { logger.Errorf("Error while cleaning network %s: %v", networkName, err) diff --git a/act/runner/run_context.go b/act/runner/run_context.go index a4c6a893..f4cacda8 100644 --- a/act/runner/run_context.go +++ b/act/runner/run_context.go @@ -52,6 +52,8 @@ type RunContext struct { Masks []string cleanUpJobContainer common.Executor caller *caller // job calling this RunContext (reusable workflows) + networkName string + networkCreated bool } func (rc *RunContext) AddMask(mask string) { @@ -97,14 +99,6 @@ func (rc *RunContext) jobContainerName() string { return createSimpleContainerName(rc.Config.ContainerNamePrefix, "WORKFLOW-"+common.Sha256(rc.Run.Workflow.Name), "JOB-"+rc.Name) } -// networkName return the name of the network which will be created by `act` automatically for job, -func (rc *RunContext) networkName() (string, bool) { - if len(rc.Run.Job().Services) > 0 || rc.Config.ContainerNetworkMode == "" { - return fmt.Sprintf("%s-%s-network", rc.jobContainerName(), rc.Run.JobID), true - } - return string(rc.Config.ContainerNetworkMode), false -} - func getDockerDaemonSocketMountPath(daemonPath string) string { if protoIndex := strings.Index(daemonPath, "://"); protoIndex != -1 { scheme := daemonPath[:protoIndex] @@ -388,15 +382,24 @@ func (rc *RunContext) startHostEnvironment() common.Executor { } } -func (rc *RunContext) getNetworkName(_ context.Context) (networkName string, createAndDeleteNetwork bool) { - // specify the network to which the container will connect when `docker create` stage. (like execute command line: docker create --network ) - networkName = string(rc.Config.ContainerNetworkMode) - if networkName == "" { - // if networkName is empty string, will create a new network for the containers. - // and it will be removed after at last. - networkName, createAndDeleteNetwork = rc.networkName() +func (rc *RunContext) getNetworkCreated(ctx context.Context) bool { + rc.ensureNetworkName(ctx) + return rc.networkCreated +} + +func (rc *RunContext) getNetworkName(ctx context.Context) string { + rc.ensureNetworkName(ctx) + return rc.networkName +} + +func (rc *RunContext) ensureNetworkName(ctx context.Context) { + if rc.networkName == "" { + rc.networkName = string(rc.Config.ContainerNetworkMode) + if len(rc.Run.Job().Services) > 0 || rc.networkName == "" { + rc.networkName = fmt.Sprintf("%s-%s-network", rc.jobContainerName(), rc.Run.JobID) + rc.networkCreated = true + } } - return networkName, createAndDeleteNetwork } var sanitizeNetworkAliasRegex = regexp.MustCompile("[^a-z0-9-]") @@ -445,7 +448,6 @@ func (rc *RunContext) prepareJobContainer(ctx context.Context) error { ext := container.LinuxContainerEnvironmentExtensions{} binds, mounts, validVolumes := rc.GetBindsAndMounts(ctx) - networkName, createAndDeleteNetwork := rc.getNetworkName(ctx) // add service containers for serviceID, spec := range rc.Run.Job().Services { // interpolate env @@ -498,7 +500,7 @@ func (rc *RunContext) prepareJobContainer(ctx context.Context) error { Privileged: rc.Config.Privileged, UsernsMode: rc.Config.UsernsMode, Platform: rc.Config.ContainerArchitecture, - NetworkMode: networkName, + NetworkMode: rc.getNetworkName(ctx), NetworkAliases: []string{sanitizeNetworkAlias(ctx, serviceID)}, ExposedPorts: exposedPorts, PortBindings: portBindings, @@ -529,13 +531,9 @@ func (rc *RunContext) prepareJobContainer(ctx context.Context) error { logger.Errorf("Error while cleaning services: %v", err) } } - if createAndDeleteNetwork { - // clean network if it has been created by act - // if using service containers - // it means that the network to which containers are connecting is created by `act_runner`, - // so, we should remove the network at last. - logger.Infof("Cleaning up network for job %s, and network name is: %s", rc.JobName, networkName) - if err := container.NewDockerNetworkRemoveExecutor(networkName)(ctx); err != nil { + if rc.getNetworkCreated(ctx) { + logger.Infof("Cleaning up network for job %s, and network name is: %s", rc.JobName, rc.getNetworkName(ctx)) + if err := container.NewDockerNetworkRemoveExecutor(rc.getNetworkName(ctx))(ctx); err != nil { logger.Errorf("Error while cleaning network: %v", err) } } @@ -556,7 +554,7 @@ func (rc *RunContext) prepareJobContainer(ctx context.Context) error { Env: envList, ToolCache: rc.getToolCache(ctx), Mounts: mounts, - NetworkMode: networkName, + NetworkMode: rc.getNetworkName(ctx), NetworkAliases: []string{sanitizeNetworkAlias(ctx, rc.Name)}, Binds: binds, Stdout: logWriter, @@ -581,7 +579,6 @@ func (rc *RunContext) startJobContainer() common.Executor { if err := rc.prepareJobContainer(ctx); err != nil { return err } - networkName, _ := rc.getNetworkName(ctx) networkConfig := network.CreateOptions{ Driver: "bridge", Scope: "local", @@ -591,8 +588,8 @@ func (rc *RunContext) startJobContainer() common.Executor { rc.pullServicesImages(rc.Config.ForcePull), rc.JobContainer.Pull(rc.Config.ForcePull), rc.stopJobContainer(), - container.NewDockerNetworkCreateExecutor(networkName, &networkConfig).IfBool(!rc.IsHostEnv(ctx) && rc.Config.ContainerNetworkMode == ""), // if the value of `ContainerNetworkMode` is empty string, then will create a new network for containers. - rc.startServiceContainers(networkName), + container.NewDockerNetworkCreateExecutor(rc.getNetworkName(ctx), &networkConfig).IfBool(!rc.IsHostEnv(ctx) && rc.Config.ContainerNetworkMode == ""), // if the value of `ContainerNetworkMode` is empty string, then will create a new network for containers. + rc.startServiceContainers(rc.getNetworkName(ctx)), rc.JobContainer.Create(rc.Config.ContainerCapAdd, rc.Config.ContainerCapDrop), rc.JobContainer.Start(false), rc.JobContainer.Copy(rc.JobContainer.GetActPath()+"/", &container.FileEntry{ diff --git a/act/runner/run_context_test.go b/act/runner/run_context_test.go index 0f70e12d..1b92a3a8 100644 --- a/act/runner/run_context_test.go +++ b/act/runner/run_context_test.go @@ -650,6 +650,88 @@ func TestRunContext_CreateSimpleContainerName(t *testing.T) { } } +func TestRunContext_ensureNetworkName(t *testing.T) { + t.Run("CreateNetworkForServices", func(t *testing.T) { + yaml := ` +on: + push: + +jobs: + job: + runs-on: docker + container: + image: some:image + services: + service1: + image: service1:image + steps: + - run: echo ok +` + workflow, err := model.ReadWorkflow(strings.NewReader(yaml), true) + require.NoError(t, err) + + rc := &RunContext{ + Config: &Config{ + ContainerNetworkMode: "host", + }, + Run: &model.Run{ + JobID: "job", + Workflow: workflow, + }, + } + + rc.ensureNetworkName(t.Context()) + assert.True(t, rc.getNetworkCreated(t.Context())) + assert.True(t, strings.HasPrefix(rc.getNetworkName(t.Context()), "WORKFLOW-"), rc.getNetworkName(t.Context())) + }) + + yaml := ` +on: + push: + +jobs: + job: + runs-on: docker + container: + image: some:image + steps: + - run: echo ok +` + workflow, err := model.ReadWorkflow(strings.NewReader(yaml), true) + require.NoError(t, err) + + run := &model.Run{ + JobID: "job", + Workflow: workflow, + } + + t.Run("CreateNetworkIfEmptyNetworkMode", func(t *testing.T) { + rc := &RunContext{ + Config: &Config{ + ContainerNetworkMode: "", + }, + Run: run, + } + + rc.ensureNetworkName(t.Context()) + assert.True(t, rc.getNetworkCreated(t.Context())) + assert.True(t, strings.HasPrefix(rc.getNetworkName(t.Context()), "WORKFLOW-"), rc.getNetworkName(t.Context())) + }) + + t.Run("FixedNetworkIfSetByNetworkMode", func(t *testing.T) { + rc := &RunContext{ + Config: &Config{ + ContainerNetworkMode: "host", + }, + Run: run, + } + + rc.ensureNetworkName(t.Context()) + assert.False(t, rc.getNetworkCreated(t.Context())) + assert.Equal(t, "host", rc.getNetworkName(t.Context())) + }) +} + func TestRunContext_SanitizeNetworkAlias(t *testing.T) { same := "same" assert.Equal(t, same, sanitizeNetworkAlias(t.Context(), same))