From 72cc81ee8dcfca5503f3a02289f8f3d4a08582ad Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 9 May 2017 11:25:33 -0400 Subject: [PATCH] Fix warning for unused build args. Signed-off-by: Daniel Nephin --- builder/dockerfile/buildargs.go | 25 +++++++++++-------- builder/dockerfile/buildargs_test.go | 36 ++++++++++++++++++++++++++++ builder/dockerfile/builder.go | 11 +-------- builder/dockerfile/dispatchers.go | 2 +- 4 files changed, 53 insertions(+), 21 deletions(-) diff --git a/builder/dockerfile/buildargs.go b/builder/dockerfile/buildargs.go index 738ee86eef..44687aaff4 100644 --- a/builder/dockerfile/buildargs.go +++ b/builder/dockerfile/buildargs.go @@ -3,6 +3,7 @@ package dockerfile import ( "fmt" "github.com/docker/docker/runconfig/opts" + "io" ) // builtinAllowedBuildArgs is list of built-in allowed build args @@ -40,16 +41,20 @@ func newBuildArgs(argsFromOptions map[string]*string) *buildArgs { } } -// UnreferencedOptionArgs returns the list of args that were set from options but -// were never referenced from the Dockerfile -func (b *buildArgs) UnreferencedOptionArgs() []string { +// WarnOnUnusedBuildArgs checks if there are any leftover build-args that were +// passed but not consumed during build. Print a warning, if there are any. +func (b *buildArgs) WarnOnUnusedBuildArgs(out io.Writer) { leftoverArgs := []string{} for arg := range b.argsFromOptions { - if _, ok := b.referencedArgs[arg]; !ok { + _, isReferenced := b.referencedArgs[arg] + _, isBuiltin := builtinAllowedBuildArgs[arg] + if !isBuiltin && !isReferenced { leftoverArgs = append(leftoverArgs, arg) } } - return leftoverArgs + if len(leftoverArgs) > 0 { + fmt.Fprintf(out, "[Warning] One or more build-args %v were not consumed\n", leftoverArgs) + } } // 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) { b.referencedArgs[key] = struct{}{} } -// IsUnreferencedBuiltin checks if the key is a built-in arg, or if it has been -// referenced by the Dockerfile. Returns true if the arg is a builtin that has -// not been referenced in the Dockerfile. -func (b *buildArgs) IsUnreferencedBuiltin(key string) bool { +// IsReferencedOrNotBuiltin checks if the key is a built-in arg, or if it has been +// referenced by the Dockerfile. Returns true if the arg is not a builtin or +// if the builtin has been referenced in the Dockerfile. +func (b *buildArgs) IsReferencedOrNotBuiltin(key string) bool { _, isBuiltin := builtinAllowedBuildArgs[key] _, isAllowed := b.allowedBuildArgs[key] - return isBuiltin && !isAllowed + return isAllowed || !isBuiltin } // GetAllAllowed returns a mapping with all the allowed args diff --git a/builder/dockerfile/buildargs_test.go b/builder/dockerfile/buildargs_test.go index 625a02b154..0288770665 100644 --- a/builder/dockerfile/buildargs_test.go +++ b/builder/dockerfile/buildargs_test.go @@ -3,6 +3,7 @@ package dockerfile import ( "testing" + "bytes" "github.com/stretchr/testify/assert" ) @@ -62,3 +63,38 @@ func TestGetAllMeta(t *testing.T) { } assert.Equal(t, expected, all) } + +func TestWarnOnUnusedBuildArgs(t *testing.T) { + buildArgs := newBuildArgs(map[string]*string{ + "ThisArgIsUsed": strPtr("fromopt1"), + "ThisArgIsNotUsed": strPtr("fromopt2"), + "HTTPS_PROXY": strPtr("referenced builtin"), + "HTTP_PROXY": strPtr("unreferenced builtin"), + }) + buildArgs.AddArg("ThisArgIsUsed", nil) + buildArgs.AddArg("HTTPS_PROXY", nil) + + buffer := new(bytes.Buffer) + buildArgs.WarnOnUnusedBuildArgs(buffer) + out := buffer.String() + assert.NotContains(t, out, "ThisArgIsUsed") + assert.NotContains(t, out, "HTTPS_PROXY") + assert.NotContains(t, out, "HTTP_PROXY") + assert.Contains(t, out, "ThisArgIsNotUsed") +} + +func TestIsUnreferencedBuiltin(t *testing.T) { + buildArgs := newBuildArgs(map[string]*string{ + "ThisArgIsUsed": strPtr("fromopt1"), + "ThisArgIsNotUsed": strPtr("fromopt2"), + "HTTPS_PROXY": strPtr("referenced builtin"), + "HTTP_PROXY": strPtr("unreferenced builtin"), + }) + buildArgs.AddArg("ThisArgIsUsed", nil) + buildArgs.AddArg("HTTPS_PROXY", nil) + + assert.True(t, buildArgs.IsReferencedOrNotBuiltin("ThisArgIsUsed")) + assert.True(t, buildArgs.IsReferencedOrNotBuiltin("ThisArgIsNotUsed")) + assert.True(t, buildArgs.IsReferencedOrNotBuiltin("HTTPS_PROXY")) + assert.False(t, buildArgs.IsReferencedOrNotBuiltin("HTTP_PROXY")) +} diff --git a/builder/dockerfile/builder.go b/builder/dockerfile/builder.go index 917e06575e..3f0e915768 100644 --- a/builder/dockerfile/builder.go +++ b/builder/dockerfile/builder.go @@ -156,7 +156,7 @@ func (b *Builder) build(source builder.Source, dockerfile *parser.Result) (*buil return nil, errors.Errorf("failed to reach build target %s in Dockerfile", b.options.Target) } - b.warnOnUnusedBuildArgs() + b.buildArgs.WarnOnUnusedBuildArgs(b.Stderr) if dispatchState.imageID == "" { 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) { dockerfile.Children = append(dockerfile.Children, node) } -// check if there are any leftover build-args that were passed but not -// consumed during build. Print a warning, if there are any. -func (b *Builder) warnOnUnusedBuildArgs() { - leftoverArgs := b.buildArgs.UnreferencedOptionArgs() - if len(leftoverArgs) > 0 { - fmt.Fprintf(b.Stderr, "[Warning] One or more build-args %v were not consumed\n", leftoverArgs) - } -} - // BuildFromConfig builds directly from `changes`, treating it as if it were the contents of a Dockerfile // It will: // - Call parse.Parse() to get an AST root for the concatenated Dockerfile entries. diff --git a/builder/dockerfile/dispatchers.go b/builder/dockerfile/dispatchers.go index 9dd8c6ab67..9491744ca9 100644 --- a/builder/dockerfile/dispatchers.go +++ b/builder/dockerfile/dispatchers.go @@ -416,7 +416,7 @@ func prependEnvOnCmd(buildArgs *buildArgs, buildArgVars []string, cmd strslice.S var tmpBuildEnv []string for _, env := range buildArgVars { key := strings.SplitN(env, "=", 2)[0] - if !buildArgs.IsUnreferencedBuiltin(key) { + if buildArgs.IsReferencedOrNotBuiltin(key) { tmpBuildEnv = append(tmpBuildEnv, env) } }