diff --git a/builder.go b/builder.go index 81d7b842..7f2f6e48 100644 --- a/builder.go +++ b/builder.go @@ -209,12 +209,8 @@ func NewStages(node *parser.Node, b *Builder) (Stages, error) { stages = append(stages, Stage{ Position: i, Name: name, - Builder: &Builder{ - Args: b.Args, - AllowedArgs: b.AllowedArgs, - Env: b.Env, - }, - Node: root, + Builder: b.builderForStage(), + Node: root, }) } return stages, nil @@ -235,17 +231,30 @@ func (b *Builder) extractHeadingArgsFromNode(node *parser.Node) error { } } + // Set children equal to everything except the leading ARG nodes + node.Children = children + + // Use a separate builder to evaluate the heading args + tempBuilder := NewBuilder(b.UserArgs) + + // Evaluate all the heading arg commands for _, c := range args { - step := b.Step() + step := tempBuilder.Step() if err := step.Resolve(c); err != nil { return err } - if err := b.Run(step, NoopExecutor, false); err != nil { + if err := tempBuilder.Run(step, NoopExecutor, false); err != nil { return err } } - node.Children = children + // Add all of the defined heading args to the original builder's HeadingArgs map + for k, v := range tempBuilder.Args { + if _, ok := tempBuilder.AllowedArgs[k]; ok { + b.HeadingArgs[k] = v + } + } + return nil } @@ -264,13 +273,23 @@ func extractNameFromNode(node *parser.Node) (string, bool) { return n.Next.Value, true } +func (b *Builder) builderForStage() *Builder { + stageBuilder := NewBuilder(b.UserArgs) + for k, v := range b.HeadingArgs { + stageBuilder.HeadingArgs[k] = v + } + return stageBuilder +} + type Builder struct { RunConfig docker.Config - Env []string - Args map[string]string - CmdSet bool - Author string + Env []string + Args map[string]string + HeadingArgs map[string]string + UserArgs map[string]string + CmdSet bool + Author string AllowedArgs map[string]bool Volumes VolumeSet @@ -288,12 +307,16 @@ func NewBuilder(args map[string]string) *Builder { for k, v := range builtinAllowedBuildArgs { allowed[k] = v } - provided := make(map[string]string) + userArgs := make(map[string]string) + initialArgs := make(map[string]string) for k, v := range args { - provided[k] = v + userArgs[k] = v + initialArgs[k] = v } return &Builder{ - Args: provided, + Args: initialArgs, + UserArgs: userArgs, + HeadingArgs: make(map[string]string), AllowedArgs: allowed, } } diff --git a/builder_test.go b/builder_test.go index 8f5369e5..5b451768 100644 --- a/builder_test.go +++ b/builder_test.go @@ -7,11 +7,14 @@ import ( "io/ioutil" "os" "reflect" + "regexp" "strings" "testing" "time" docker "github.com/fsouza/go-dockerclient" + + "github.com/openshift/imagebuilder/dockerfile/parser" ) func TestVolumeSet(t *testing.T) { @@ -159,6 +162,19 @@ func TestMultiStageParseHeadingArg(t *testing.T) { if len(stages) != 3 { t.Fatalf("expected 3 stages, got %d", len(stages)) } + + fromImages := []string{"golang:1.9", "busybox:latest", "golang:1.9"} + for i, stage := range stages { + from, err := stage.Builder.From(stage.Node) + if err != nil { + t.Fatal(err) + } + + if expected := fromImages[i]; from != expected { + t.Fatalf("expected %s, got %s", expected, from) + } + } + t.Logf("stages: %#v", stages) } @@ -192,6 +208,216 @@ RUN echo $FOO $BAR`)) } } +func resolveNodeArgs(b *Builder, node *parser.Node) error { + for _, c := range node.Children { + if c.Value != "arg" { + continue + } + step := b.Step() + if err := step.Resolve(c); err != nil { + return err + } + if err := b.Run(step, NoopExecutor, false); err != nil { + return err + } + } + return nil +} + +func builderHasArgument(b *Builder, argString string) bool { + for _, arg := range b.Arguments() { + if arg == argString { + return true + } + } + return false +} + +func TestMultiStageHeadingArgRedefine(t *testing.T) { + n, err := ParseFile("dockerclient/testdata/multistage/Dockerfile.heading-redefine") + if err != nil { + t.Fatal(err) + } + stages, err := NewStages(n, NewBuilder(map[string]string{})) + if err != nil { + t.Fatal(err) + } + if len(stages) != 2 { + t.Fatalf("expected 2 stages, got %d", len(stages)) + } + + for _, stage := range stages { + if err := resolveNodeArgs(stage.Builder, stage.Node); err != nil { + t.Fatal(err) + } + } + + firstStageHasArg := false + for _, arg := range stages[0].Builder.Arguments() { + if match, err := regexp.MatchString(`FOO=.*`, arg); err == nil && match { + firstStageHasArg = true + break + } else if err != nil { + t.Fatal(err) + } + } + if firstStageHasArg { + t.Fatalf("expected FOO to not be present in first stage") + } + + if !builderHasArgument(stages[1].Builder, "FOO=latest") { + t.Fatalf("expected FOO=latest in second stage arguments list, got %v", stages[1].Builder.Arguments()) + } +} + +func TestMultiStageHeadingArgRedefineOverride(t *testing.T) { + n, err := ParseFile("dockerclient/testdata/multistage/Dockerfile.heading-redefine") + if err != nil { + t.Fatal(err) + } + stages, err := NewStages(n, NewBuilder(map[string]string{"FOO": "7"})) + if err != nil { + t.Fatal(err) + } + if len(stages) != 2 { + t.Fatalf("expected 2 stages, got %d", len(stages)) + } + + for _, stage := range stages { + if err := resolveNodeArgs(stage.Builder, stage.Node); err != nil { + t.Fatal(err) + } + } + + firstStageHasArg := false + for _, arg := range stages[0].Builder.Arguments() { + if match, err := regexp.MatchString(`FOO=.*`, arg); err == nil && match { + firstStageHasArg = true + break + } else if err != nil { + t.Fatal(err) + } + } + if firstStageHasArg { + t.Fatalf("expected FOO to not be present in first stage") + } + + if !builderHasArgument(stages[1].Builder, "FOO=7") { + t.Fatalf("expected FOO=7 in second stage arguments list, got %v", stages[1].Builder.Arguments()) + } +} + +func TestArgs(t *testing.T) { + for _, tc := range []struct { + name string + dockerfile string + args map[string]string + expectedValue string + }{ + { + name: "argOverride", + dockerfile: "FROM centos\nARG FOO=stuff\nARG FOO=things\n", + args: map[string]string{}, + expectedValue: "FOO=things", + }, + { + name: "argOverrideWithBuildArgs", + dockerfile: "FROM centos\nARG FOO=stuff\nARG FOO=things\n", + args: map[string]string{"FOO": "bar"}, + expectedValue: "FOO=bar", + }, + { + name: "headingArgRedefine", + dockerfile: "ARG FOO=stuff\nFROM centos\nARG FOO\n", + args: map[string]string{}, + expectedValue: "FOO=stuff", + }, + { + name: "headingArgRedefineWithBuildArgs", + dockerfile: "ARG FOO=stuff\nFROM centos\nARG FOO\n", + args: map[string]string{"FOO": "bar"}, + expectedValue: "FOO=bar", + }, + { + name: "headingArgRedefineDefault", + dockerfile: "ARG FOO=stuff\nFROM centos\nARG FOO=defaultfoovalue\n", + args: map[string]string{}, + expectedValue: "FOO=defaultfoovalue", + }, + { + name: "headingArgRedefineDefaultWithBuildArgs", + dockerfile: "ARG FOO=stuff\nFROM centos\nARG FOO=defaultfoovalue\n", + args: map[string]string{"FOO": "bar"}, + expectedValue: "FOO=bar", + }, + } { + t.Run(tc.name, func(t *testing.T) { + node, err := ParseDockerfile(strings.NewReader(tc.dockerfile)) + if err != nil { + t.Fatal(err) + } + + b := NewBuilder(tc.args) + if err := resolveNodeArgs(b, node); err != nil { + t.Fatal(err) + } + + if !builderHasArgument(b, tc.expectedValue) { + t.Fatalf("expected %s to be contained in arguments list: %v", tc.expectedValue, b.Arguments()) + } + }) + } +} + +func TestMultiStageArgScope(t *testing.T) { + n, err := ParseFile("dockerclient/testdata/multistage/Dockerfile.arg-scope") + if err != nil { + t.Fatal(err) + } + args := map[string]string{ + "SECRET": "secretthings", + "BAR": "notsecretthings", + } + stages, err := NewStages(n, NewBuilder(args)) + if err != nil { + t.Fatal(err) + } + if len(stages) != 2 { + t.Fatalf("expected 2 stages, got %d", len(stages)) + } + + for _, stage := range stages { + if err := resolveNodeArgs(stage.Builder, stage.Node); err != nil { + t.Fatal(err) + } + } + + if !builderHasArgument(stages[0].Builder, "SECRET=secretthings") { + t.Fatalf("expected SECRET=secretthings to be contained in first stage arguments list: %v", stages[0].Builder.Arguments()) + } + + secondStageArguments := stages[1].Builder.Arguments() + secretInSecondStage := false + for _, arg := range secondStageArguments { + if match, err := regexp.MatchString(`SECRET=.*`, arg); err == nil && match { + secretInSecondStage = true + break + } else if err != nil { + t.Fatal(err) + } + } + if secretInSecondStage { + t.Fatalf("expected SECRET to not be present in second stage") + } + + if !builderHasArgument(stages[1].Builder, "FOO=test") { + t.Fatalf("expected FOO=test to be present in second stage arguments list: %v", secondStageArguments) + } + if !builderHasArgument(stages[1].Builder, "BAR=notsecretthings") { + t.Fatalf("expected BAR=notsecretthings to be present in second stage arguments list: %v", secondStageArguments) + } +} + func TestRun(t *testing.T) { f, err := os.Open("dockerclient/testdata/Dockerfile.add") if err != nil { diff --git a/dispatchers.go b/dispatchers.go index e7f2f97b..1d77a193 100644 --- a/dispatchers.go +++ b/dispatchers.go @@ -216,7 +216,7 @@ func from(b *Builder, args []string, attributes map[string]bool, flagArgs []stri // Support ARG before from argStrs := []string{} - for n, v := range b.Args { + for n, v := range b.HeadingArgs { argStrs = append(argStrs, n+"="+v) } var err error @@ -598,10 +598,16 @@ func arg(b *Builder, args []string, attributes map[string]bool, flagArgs []strin // add the arg to allowed list of build-time args from this step on. b.AllowedArgs[name] = true + // If there is still no default value, a value can be assigned from the heading args + if val, ok := b.HeadingArgs[name]; ok && !hasDefault { + b.Args[name] = val + } + // If there is a default value associated with this arg then add it to the - // b.buildArgs if one is not already passed to the builder. The args passed - // to builder override the default value of 'arg'. - if _, ok := b.Args[name]; !ok && hasDefault { + // b.buildArgs, later default values for the same arg override earlier ones. + // The args passed to builder (UserArgs) override the default value of 'arg' + // Don't add them here as they were already set in NewBuilder. + if _, ok := b.UserArgs[name]; !ok && hasDefault { b.Args[name] = value } diff --git a/dockerclient/testdata/multistage/Dockerfile.arg-scope b/dockerclient/testdata/multistage/Dockerfile.arg-scope new file mode 100644 index 00000000..1c3e088d --- /dev/null +++ b/dockerclient/testdata/multistage/Dockerfile.arg-scope @@ -0,0 +1,9 @@ +FROM alpine +ARG SECRET +RUN echo "$SECRET" + +FROM alpine +ARG FOO=test +ARG BAR=bartest +RUN echo "$FOO:$BAR" +RUN echo "$SECRET" diff --git a/dockerclient/testdata/multistage/Dockerfile.heading-arg b/dockerclient/testdata/multistage/Dockerfile.heading-arg index 54ee396e..76ccffe1 100644 --- a/dockerclient/testdata/multistage/Dockerfile.heading-arg +++ b/dockerclient/testdata/multistage/Dockerfile.heading-arg @@ -1,5 +1,6 @@ ARG GO_VERSION=1.9 -FROM golang:$GO_VERSION as builder +ARG GO_IMAGE=golang +FROM $GO_IMAGE:$GO_VERSION as builder ARG FOO WORKDIR /tmp COPY . . @@ -10,7 +11,7 @@ WORKDIR /tmp COPY --from=builder /tmp/bar /tmp/bar RUN echo foo2 >> /tmp/bar -FROM busybox:latest +FROM $GO_IMAGE:$GO_VERSION WORKDIR / COPY --from=modifier /tmp/bar /bin/baz diff --git a/dockerclient/testdata/multistage/Dockerfile.heading-redefine b/dockerclient/testdata/multistage/Dockerfile.heading-redefine new file mode 100644 index 00000000..4d476f36 --- /dev/null +++ b/dockerclient/testdata/multistage/Dockerfile.heading-redefine @@ -0,0 +1,7 @@ +ARG FOO=latest +FROM alpine +RUN echo "$FOO" + +FROM centos:$FOO +ARG FOO +RUN echo "$FOO"