Browse code

Fix warning for unused build args.

Signed-off-by: Daniel Nephin <dnephin@docker.com>

Daniel Nephin authored on 2017/05/10 00:25:33
Showing 4 changed files
... ...
@@ -3,6 +3,7 @@ package dockerfile
3 3
 import (
4 4
 	"fmt"
5 5
 	"github.com/docker/docker/runconfig/opts"
6
+	"io"
6 7
 )
7 8
 
8 9
 // builtinAllowedBuildArgs is list of built-in allowed build args
... ...
@@ -40,16 +41,20 @@ func newBuildArgs(argsFromOptions map[string]*string) *buildArgs {
40 40
 	}
41 41
 }
42 42
 
43
-// UnreferencedOptionArgs returns the list of args that were set from options but
44
-// were never referenced from the Dockerfile
45
-func (b *buildArgs) UnreferencedOptionArgs() []string {
43
+// WarnOnUnusedBuildArgs checks if there are any leftover build-args that were
44
+// passed but not consumed during build. Print a warning, if there are any.
45
+func (b *buildArgs) WarnOnUnusedBuildArgs(out io.Writer) {
46 46
 	leftoverArgs := []string{}
47 47
 	for arg := range b.argsFromOptions {
48
-		if _, ok := b.referencedArgs[arg]; !ok {
48
+		_, isReferenced := b.referencedArgs[arg]
49
+		_, isBuiltin := builtinAllowedBuildArgs[arg]
50
+		if !isBuiltin && !isReferenced {
49 51
 			leftoverArgs = append(leftoverArgs, arg)
50 52
 		}
51 53
 	}
52
-	return leftoverArgs
54
+	if len(leftoverArgs) > 0 {
55
+		fmt.Fprintf(out, "[Warning] One or more build-args %v were not consumed\n", leftoverArgs)
56
+	}
53 57
 }
54 58
 
55 59
 // ResetAllowed clears the list of args that are allowed to be used by a
... ...
@@ -69,13 +74,13 @@ func (b *buildArgs) AddArg(key string, value *string) {
69 69
 	b.referencedArgs[key] = struct{}{}
70 70
 }
71 71
 
72
-// IsUnreferencedBuiltin checks if the key is a built-in arg, or if it has been
73
-// referenced by the Dockerfile. Returns true if the arg is a builtin that has
74
-// not been referenced in the Dockerfile.
75
-func (b *buildArgs) IsUnreferencedBuiltin(key string) bool {
72
+// IsReferencedOrNotBuiltin checks if the key is a built-in arg, or if it has been
73
+// referenced by the Dockerfile. Returns true if the arg is not a builtin or
74
+// if the builtin has been referenced in the Dockerfile.
75
+func (b *buildArgs) IsReferencedOrNotBuiltin(key string) bool {
76 76
 	_, isBuiltin := builtinAllowedBuildArgs[key]
77 77
 	_, isAllowed := b.allowedBuildArgs[key]
78
-	return isBuiltin && !isAllowed
78
+	return isAllowed || !isBuiltin
79 79
 }
80 80
 
81 81
 // GetAllAllowed returns a mapping with all the allowed args
... ...
@@ -3,6 +3,7 @@ package dockerfile
3 3
 import (
4 4
 	"testing"
5 5
 
6
+	"bytes"
6 7
 	"github.com/stretchr/testify/assert"
7 8
 )
8 9
 
... ...
@@ -62,3 +63,38 @@ func TestGetAllMeta(t *testing.T) {
62 62
 	}
63 63
 	assert.Equal(t, expected, all)
64 64
 }
65
+
66
+func TestWarnOnUnusedBuildArgs(t *testing.T) {
67
+	buildArgs := newBuildArgs(map[string]*string{
68
+		"ThisArgIsUsed":    strPtr("fromopt1"),
69
+		"ThisArgIsNotUsed": strPtr("fromopt2"),
70
+		"HTTPS_PROXY":      strPtr("referenced builtin"),
71
+		"HTTP_PROXY":       strPtr("unreferenced builtin"),
72
+	})
73
+	buildArgs.AddArg("ThisArgIsUsed", nil)
74
+	buildArgs.AddArg("HTTPS_PROXY", nil)
75
+
76
+	buffer := new(bytes.Buffer)
77
+	buildArgs.WarnOnUnusedBuildArgs(buffer)
78
+	out := buffer.String()
79
+	assert.NotContains(t, out, "ThisArgIsUsed")
80
+	assert.NotContains(t, out, "HTTPS_PROXY")
81
+	assert.NotContains(t, out, "HTTP_PROXY")
82
+	assert.Contains(t, out, "ThisArgIsNotUsed")
83
+}
84
+
85
+func TestIsUnreferencedBuiltin(t *testing.T) {
86
+	buildArgs := newBuildArgs(map[string]*string{
87
+		"ThisArgIsUsed":    strPtr("fromopt1"),
88
+		"ThisArgIsNotUsed": strPtr("fromopt2"),
89
+		"HTTPS_PROXY":      strPtr("referenced builtin"),
90
+		"HTTP_PROXY":       strPtr("unreferenced builtin"),
91
+	})
92
+	buildArgs.AddArg("ThisArgIsUsed", nil)
93
+	buildArgs.AddArg("HTTPS_PROXY", nil)
94
+
95
+	assert.True(t, buildArgs.IsReferencedOrNotBuiltin("ThisArgIsUsed"))
96
+	assert.True(t, buildArgs.IsReferencedOrNotBuiltin("ThisArgIsNotUsed"))
97
+	assert.True(t, buildArgs.IsReferencedOrNotBuiltin("HTTPS_PROXY"))
98
+	assert.False(t, buildArgs.IsReferencedOrNotBuiltin("HTTP_PROXY"))
99
+}
... ...
@@ -156,7 +156,7 @@ func (b *Builder) build(source builder.Source, dockerfile *parser.Result) (*buil
156 156
 		return nil, errors.Errorf("failed to reach build target %s in Dockerfile", b.options.Target)
157 157
 	}
158 158
 
159
-	b.warnOnUnusedBuildArgs()
159
+	b.buildArgs.WarnOnUnusedBuildArgs(b.Stderr)
160 160
 
161 161
 	if dispatchState.imageID == "" {
162 162
 		return nil, errors.New("No image was generated. Is your Dockerfile empty?")
... ...
@@ -235,15 +235,6 @@ func addNodesForLabelOption(dockerfile *parser.Node, labels map[string]string) {
235 235
 	dockerfile.Children = append(dockerfile.Children, node)
236 236
 }
237 237
 
238
-// check if there are any leftover build-args that were passed but not
239
-// consumed during build. Print a warning, if there are any.
240
-func (b *Builder) warnOnUnusedBuildArgs() {
241
-	leftoverArgs := b.buildArgs.UnreferencedOptionArgs()
242
-	if len(leftoverArgs) > 0 {
243
-		fmt.Fprintf(b.Stderr, "[Warning] One or more build-args %v were not consumed\n", leftoverArgs)
244
-	}
245
-}
246
-
247 238
 // BuildFromConfig builds directly from `changes`, treating it as if it were the contents of a Dockerfile
248 239
 // It will:
249 240
 // - Call parse.Parse() to get an AST root for the concatenated Dockerfile entries.
... ...
@@ -416,7 +416,7 @@ func prependEnvOnCmd(buildArgs *buildArgs, buildArgVars []string, cmd strslice.S
416 416
 	var tmpBuildEnv []string
417 417
 	for _, env := range buildArgVars {
418 418
 		key := strings.SplitN(env, "=", 2)[0]
419
-		if !buildArgs.IsUnreferencedBuiltin(key) {
419
+		if buildArgs.IsReferencedOrNotBuiltin(key) {
420 420
 			tmpBuildEnv = append(tmpBuildEnv, env)
421 421
 		}
422 422
 	}