Merge pull request #151 from carbonin/scope_args_to_stage

Treat args the same way `docker build` does

Author: openshift-merge-robot

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,
 	}
 }

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 {

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
 	}
 

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"