From f32babb51db6761fd5dff1bcc03bbdf9eb7aa354 Mon Sep 17 00:00:00 2001 From: "Ryan (hackercat)" Date: Sun, 2 May 2021 17:15:13 +0200 Subject: [PATCH] fix: reworked container architecture (#619) - Don't set architecture, let Docker host decide it's own platform, remove `runtime` dependency and don't show default in `--help` - Remove most tests, we need to check only once if it works on different platform - Rename `DeleteImage` to `RemoveImage` to conform to existing function in `docker` cli, added options to specify `force` and `pruneChildren` --- README.md | 2 +- cmd/root.go | 3 +- pkg/container/docker_images.go | 10 ++-- pkg/container/docker_run.go | 2 +- pkg/runner/run_context.go | 4 -- pkg/runner/runner_test.go | 57 ++++++++--------------- pkg/runner/step_context.go | 82 ++++++++++++++++----------------- pkg/runner/step_context_test.go | 16 ++----- 8 files changed, 73 insertions(+), 103 deletions(-) diff --git a/README.md b/README.md index 5bc30c7..f166d77 100644 --- a/README.md +++ b/README.md @@ -88,7 +88,7 @@ It will save that information to `~/.actrc`, please refer to [Configuration](#co ```none -a, --actor string user that triggered the event (default "nektos/act") -b, --bind bind working directory to container, rather than copy - --container-architecture string Architecture which should be used to run containers, e.g.: linux/amd64. Defaults to linux/ [linux/amd64]. Requires Docker server API Version 1.41+. Ignored on earlier Docker server platforms. + --container-architecture string Architecture which should be used to run containers, e.g.: linux/amd64. If not specified, will use host default architecture. Requires Docker server API Version 1.41+. Ignored on earlier Docker server platforms. --defaultbranch string the name of the main branch --detect-event Use first event type from workflow as event that triggered the workflow -C, --directory string working directory (default ".") diff --git a/cmd/root.go b/cmd/root.go index 8dd08f4..8ef464a 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -6,7 +6,6 @@ import ( "os" "path/filepath" "regexp" - "runtime" "strings" "github.com/nektos/act/pkg/common" @@ -59,7 +58,7 @@ func Execute(ctx context.Context, version string) { rootCmd.PersistentFlags().StringVarP(&input.secretfile, "secret-file", "", ".secrets", "file with list of secrets to read from (e.g. --secret-file .secrets)") rootCmd.PersistentFlags().BoolVarP(&input.insecureSecrets, "insecure-secrets", "", false, "NOT RECOMMENDED! Doesn't hide secrets while printing logs.") rootCmd.PersistentFlags().StringVarP(&input.envfile, "env-file", "", ".env", "environment file to read and use as env in the containers") - rootCmd.PersistentFlags().StringVarP(&input.containerArchitecture, "container-architecture", "", "", "Architecture which should be used to run containers, e.g.: linux/amd64. Defaults to linux/ [linux/"+runtime.GOARCH+"]. Requires Docker server API Version 1.41+. Ignored on earlier Docker server platforms.") + rootCmd.PersistentFlags().StringVarP(&input.containerArchitecture, "container-architecture", "", "", "Architecture which should be used to run containers, e.g.: linux/amd64. If not specified, will use host default architecture. Requires Docker server API Version 1.41+. Ignored on earlier Docker server platforms.") rootCmd.SetArgs(args()) if err := rootCmd.Execute(); err != nil { diff --git a/pkg/container/docker_images.go b/pkg/container/docker_images.go index 37eaec5..f9dab3c 100644 --- a/pkg/container/docker_images.go +++ b/pkg/container/docker_images.go @@ -29,7 +29,7 @@ func ImageExistsLocally(ctx context.Context, imageName string, platform string) } if len(images) > 0 { - if platform == "any" { + if platform == "any" || platform == "" { return true, nil } for _, v := range images { @@ -48,9 +48,9 @@ func ImageExistsLocally(ctx context.Context, imageName string, platform string) return false, nil } -// DeleteImage removes image from local store, the function is used to run different +// RemoveImage removes image from local store, the function is used to run different // container image architectures -func DeleteImage(ctx context.Context, imageName string) (bool, error) { +func RemoveImage(ctx context.Context, imageName string, force bool, pruneChildren bool) (bool, error) { if exists, err := ImageExistsLocally(ctx, imageName, "any"); !exists { return false, err } @@ -75,8 +75,8 @@ func DeleteImage(ctx context.Context, imageName string) (bool, error) { if len(images) > 0 { for _, v := range images { if _, err = cli.ImageRemove(ctx, v.ID, types.ImageRemoveOptions{ - Force: true, - PruneChildren: true, + Force: force, + PruneChildren: pruneChildren, }); err != nil { return false, err } diff --git a/pkg/container/docker_run.go b/pkg/container/docker_run.go index 0bbe42d..cec98cb 100644 --- a/pkg/container/docker_run.go +++ b/pkg/container/docker_run.go @@ -293,7 +293,7 @@ func (cr *containerReference) create() common.Executor { } var platSpecs *specs.Platform - if supportsContainerImagePlatform(cr.cli) { + if supportsContainerImagePlatform(cr.cli) && cr.input.Platform != "" { desiredPlatform := strings.SplitN(cr.input.Platform, `/`, 2) if len(desiredPlatform) != 2 { diff --git a/pkg/runner/run_context.go b/pkg/runner/run_context.go index 7939986..042fb2f 100755 --- a/pkg/runner/run_context.go +++ b/pkg/runner/run_context.go @@ -96,10 +96,6 @@ func (rc *RunContext) startJobContainer() common.Executor { binds = append(binds, fmt.Sprintf("%s:%s%s", rc.Config.Workdir, rc.Config.Workdir, bindModifiers)) } - if rc.Config.ContainerArchitecture == "" { - rc.Config.ContainerArchitecture = fmt.Sprintf("%s/%s", "linux", runtime.GOARCH) - } - rc.JobContainer = container.NewContainer(&container.NewContainerInput{ Cmd: nil, Entrypoint: []string{"/usr/bin/tail", "-f", "/dev/null"}, diff --git a/pkg/runner/runner_test.go b/pkg/runner/runner_test.go index 7e15d0a..fc35f0e 100755 --- a/pkg/runner/runner_test.go +++ b/pkg/runner/runner_test.go @@ -79,45 +79,28 @@ func TestRunEvent(t *testing.T) { "ubuntu-latest": "node:12.20.1-buster-slim", } tables := []TestJobFileInfo{ - // {"testdata", "powershell", "push", "", platforms}, // Powershell is not available on default act test runner (yet) but preserving here for posterity - {"testdata", "basic", "push", "", platforms, "linux/amd64"}, - {"testdata", "fail", "push", "exit with `FAILURE`: 1", platforms, "linux/amd64"}, - {"testdata", "runs-on", "push", "", platforms, "linux/amd64"}, - {"testdata", "job-container", "push", "", platforms, "linux/amd64"}, - {"testdata", "job-container-non-root", "push", "", platforms, "linux/amd64"}, - {"testdata", "uses-docker-url", "push", "", platforms, "linux/amd64"}, - {"testdata", "remote-action-docker", "push", "", platforms, "linux/amd64"}, - {"testdata", "remote-action-js", "push", "", platforms, "linux/amd64"}, - {"testdata", "local-action-docker-url", "push", "", platforms, "linux/amd64"}, - {"testdata", "local-action-dockerfile", "push", "", platforms, "linux/amd64"}, - {"testdata", "local-action-js", "push", "", platforms, "linux/amd64"}, - {"testdata", "matrix", "push", "", platforms, "linux/amd64"}, - {"testdata", "matrix-include-exclude", "push", "", platforms, "linux/amd64"}, - {"testdata", "commands", "push", "", platforms, "linux/amd64"}, - {"testdata", "workdir", "push", "", platforms, "linux/amd64"}, - // {"testdata", "issue-228", "push", "", platforms, "linux/amd64"}, // TODO [igni]: Remove this once everything passes - {"testdata", "defaults-run", "push", "", platforms, "linux/amd64"}, - {"testdata", "uses-composite", "push", "", platforms, "linux/amd64"}, + {"testdata", "basic", "push", "", platforms, ""}, + {"testdata", "fail", "push", "exit with `FAILURE`: 1", platforms, ""}, + {"testdata", "runs-on", "push", "", platforms, ""}, + {"testdata", "job-container", "push", "", platforms, ""}, + {"testdata", "job-container-non-root", "push", "", platforms, ""}, + {"testdata", "uses-docker-url", "push", "", platforms, ""}, + {"testdata", "remote-action-docker", "push", "", platforms, ""}, + {"testdata", "remote-action-js", "push", "", platforms, ""}, + {"testdata", "local-action-docker-url", "push", "", platforms, ""}, + {"testdata", "local-action-dockerfile", "push", "", platforms, ""}, + {"testdata", "local-action-js", "push", "", platforms, ""}, + {"testdata", "matrix", "push", "", platforms, ""}, + {"testdata", "matrix-include-exclude", "push", "", platforms, ""}, + {"testdata", "commands", "push", "", platforms, ""}, + {"testdata", "workdir", "push", "", platforms, ""}, + {"testdata", "defaults-run", "push", "", platforms, ""}, + {"testdata", "uses-composite", "push", "", platforms, ""}, + // {"testdata", "powershell", "push", "", platforms, ""}, // Powershell is not available on default act test runner (yet) but preserving here for posterity + // {"testdata", "issue-228", "push", "", platforms, ""}, // TODO [igni]: Remove this once everything passes - // linux/arm64 + // single test for different architecture: linux/arm64 {"testdata", "basic", "push", "", platforms, "linux/arm64"}, - {"testdata", "fail", "push", "exit with `FAILURE`: 1", platforms, "linux/arm64"}, - {"testdata", "runs-on", "push", "", platforms, "linux/arm64"}, - {"testdata", "job-container", "push", "", platforms, "linux/arm64"}, - {"testdata", "job-container-non-root", "push", "", platforms, "linux/arm64"}, - {"testdata", "uses-docker-url", "push", "", platforms, "linux/arm64"}, - {"testdata", "remote-action-docker", "push", "", platforms, "linux/arm64"}, - {"testdata", "remote-action-js", "push", "", platforms, "linux/arm64"}, - {"testdata", "local-action-docker-url", "push", "", platforms, "linux/arm64"}, - {"testdata", "local-action-dockerfile", "push", "", platforms, "linux/arm64"}, - {"testdata", "local-action-js", "push", "", platforms, "linux/arm64"}, - {"testdata", "matrix", "push", "", platforms, "linux/arm64"}, - {"testdata", "matrix-include-exclude", "push", "", platforms, "linux/arm64"}, - {"testdata", "commands", "push", "", platforms, "linux/arm64"}, - {"testdata", "workdir", "push", "", platforms, "linux/arm64"}, - // {"testdata", "issue-228", "push", "", platforms, "linux/arm64"}, // TODO [igni]: Remove this once everything passes - {"testdata", "defaults-run", "push", "", platforms, "linux/arm64"}, - {"testdata", "uses-composite", "push", "", platforms, "linux/arm64"}, } log.SetLevel(log.DebugLevel) diff --git a/pkg/runner/step_context.go b/pkg/runner/step_context.go index f8bb7bc..9e49a68 100755 --- a/pkg/runner/step_context.go +++ b/pkg/runner/step_context.go @@ -252,10 +252,6 @@ func (sc *StepContext) newStepContainer(ctx context.Context, image string, cmd [ binds = append(binds, fmt.Sprintf("%s:%s%s", rc.Config.Workdir, rc.Config.Workdir, bindModifiers)) } - if rc.Config.ContainerArchitecture == "" { - rc.Config.ContainerArchitecture = fmt.Sprintf("%s/%s", "linux", runtime.GOARCH) - } - stepContainer := container.NewContainer(&container.NewContainerInput{ Cmd: cmd, Entrypoint: entrypoint, @@ -438,33 +434,35 @@ func (sc *StepContext) runAction(actionDir string, actionPath string) common.Exe image = strings.ToLower(image) contextDir := filepath.Join(actionDir, actionPath, action.Runs.Main) - exists, err := container.ImageExistsLocally(ctx, image, "any") + anyArchExists, err := container.ImageExistsLocally(ctx, image, "any") if err != nil { return err } - if exists { - wasRemoved, err := container.DeleteImage(ctx, image) + correctArchExists, err := container.ImageExistsLocally(ctx, image, rc.Config.ContainerArchitecture) + if err != nil { + return err + } + + if anyArchExists && !correctArchExists { + wasRemoved, err := container.RemoveImage(ctx, image, true, true) if err != nil { return err } if !wasRemoved { - return fmt.Errorf("failed to delete image '%s'", image) + return fmt.Errorf("failed to remove image '%s'", image) } } - prepImage = container.NewDockerBuildExecutor(container.NewDockerBuildExecutorInput{ - ContextDir: contextDir, - ImageTag: image, - Platform: rc.Config.ContainerArchitecture, - }) - exists, err = container.ImageExistsLocally(ctx, image, rc.Config.ContainerArchitecture) - if err != nil { - return err - } - - if !exists { - return err + if !correctArchExists { + log.Debugf("image '%s' for architecture '%s' will be built from context '%s", image, rc.Config.ContainerArchitecture, contextDir) + prepImage = container.NewDockerBuildExecutor(container.NewDockerBuildExecutorInput{ + ContextDir: contextDir, + ImageTag: image, + Platform: rc.Config.ContainerArchitecture, + }) + } else { + log.Debugf("image '%s' for architecture '%s' already exists", image, rc.Config.ContainerArchitecture) } } @@ -508,27 +506,27 @@ func (sc *StepContext) runAction(actionDir string, actionPath string) common.Exe stepID := 0 for _, compositeStep := range action.Runs.Steps { stepClone := compositeStep - // Take a copy of the run context structure (rc is a pointer) - // Then take the address of the new structure - rcCloneStr := *rc - rcClone := &rcCloneStr + // Take a copy of the run context structure (rc is a pointer) + // Then take the address of the new structure + rcCloneStr := *rc + rcClone := &rcCloneStr if stepClone.ID == "" { stepClone.ID = fmt.Sprintf("composite-%d", stepID) stepID++ } - rcClone.CurrentStep = stepClone.ID + rcClone.CurrentStep = stepClone.ID - if err := compositeStep.Validate(); err != nil { - return err - } + if err := compositeStep.Validate(); err != nil { + return err + } - // Setup the outputs for the composite steps - if _, ok := rcClone.StepResults[stepClone.ID]; ! ok { - rcClone.StepResults[stepClone.ID] = &stepResult{ - Success: true, - Outputs: make(map[string]string), - } - } + // Setup the outputs for the composite steps + if _, ok := rcClone.StepResults[stepClone.ID]; !ok { + rcClone.StepResults[stepClone.ID] = &stepResult{ + Success: true, + Outputs: make(map[string]string), + } + } stepClone.Run = strings.ReplaceAll(stepClone.Run, "${{ github.action_path }}", filepath.Join(containerActionDir, actionName)) @@ -538,14 +536,14 @@ func (sc *StepContext) runAction(actionDir string, actionPath string) common.Exe Env: mergeMaps(sc.Env, stepClone.Env), } - // Interpolate the outer inputs into the composite step with items - exprEval := sc.NewExpressionEvaluator() - for k, v := range stepContext.Step.With { + // Interpolate the outer inputs into the composite step with items + exprEval := sc.NewExpressionEvaluator() + for k, v := range stepContext.Step.With { - if strings.Contains(v, "inputs") { - stepContext.Step.With[k] = exprEval.Interpolate(v) - } - } + if strings.Contains(v, "inputs") { + stepContext.Step.With[k] = exprEval.Interpolate(v) + } + } executors = append(executors, stepContext.Executor()) } diff --git a/pkg/runner/step_context_test.go b/pkg/runner/step_context_test.go index 920f43e..7cb2d1f 100644 --- a/pkg/runner/step_context_test.go +++ b/pkg/runner/step_context_test.go @@ -12,17 +12,11 @@ func TestStepContextExecutor(t *testing.T) { "ubuntu-latest": "node:12.20.1-buster-slim", } tables := []TestJobFileInfo{ - {"testdata", "uses-and-run-in-one-step", "push", "Invalid run/uses syntax for job:test step:Test", platforms, "linux/amd64"}, - {"testdata", "uses-github-empty", "push", "Expected format {org}/{repo}[/path]@ref", platforms, "linux/amd64"}, - {"testdata", "uses-github-noref", "push", "Expected format {org}/{repo}[/path]@ref", platforms, "linux/amd64"}, - {"testdata", "uses-github-root", "push", "", platforms, "linux/amd64"}, - {"testdata", "uses-github-path", "push", "", platforms, "linux/amd64"}, - - {"testdata", "uses-and-run-in-one-step", "push", "Invalid run/uses syntax for job:test step:Test", platforms, "linux/arm64"}, - {"testdata", "uses-github-empty", "push", "Expected format {org}/{repo}[/path]@ref", platforms, "linux/arm64"}, - {"testdata", "uses-github-noref", "push", "Expected format {org}/{repo}[/path]@ref", platforms, "linux/arm64"}, - {"testdata", "uses-github-root", "push", "", platforms, "linux/arm64"}, - {"testdata", "uses-github-path", "push", "", platforms, "linux/arm64"}, + {"testdata", "uses-and-run-in-one-step", "push", "Invalid run/uses syntax for job:test step:Test", platforms, ""}, + {"testdata", "uses-github-empty", "push", "Expected format {org}/{repo}[/path]@ref", platforms, ""}, + {"testdata", "uses-github-noref", "push", "Expected format {org}/{repo}[/path]@ref", platforms, ""}, + {"testdata", "uses-github-root", "push", "", platforms, ""}, + {"testdata", "uses-github-path", "push", "", platforms, ""}, } // These tests are sufficient to only check syntax. ctx := common.WithDryrun(context.Background(), true)