From 30a4b38ae2790a292afbb597ac88735ec75d0f8b Mon Sep 17 00:00:00 2001 From: Talon Bowler Date: Wed, 24 Apr 2024 10:02:23 -0700 Subject: [PATCH] update lint tests to accept a struct instead of an increasing number of parameters Signed-off-by: Talon Bowler --- frontend/dockerfile/dockerfile2llb/convert.go | 49 +++++++++---------- frontend/dockerfile/dockerfile_lint_test.go | 39 +++++++++++++-- frontend/dockerfile/shell/lex.go | 4 +- frontend/dockerfile/shell/lex_test.go | 9 ++-- 4 files changed, 66 insertions(+), 35 deletions(-) diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go index fd0dc09eb7619..b40576c67e361 100644 --- a/frontend/dockerfile/dockerfile2llb/convert.go +++ b/frontend/dockerfile/dockerfile2llb/convert.go @@ -237,7 +237,7 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS if v, ok := opt.BuildArgs[metaArg.Key]; !ok { if metaArg.Value != nil { result, _ := shlex.ProcessWordWithMatches(*metaArg.Value, metaArgsToMap(optMetaArgs)) - *metaArg.Value = result.Word + *metaArg.Value = result.Result info.deps = result.Matched } } else { @@ -260,22 +260,17 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS // set base state for every image for i, st := range stages { - result, err := shlex.ProcessWordWithMatches(st.BaseName, metaArgsToMap(optMetaArgs)) - name := result.Word - used := result.Matched - if len(result.Unmatched) > 0 { - for unmatched := range result.Unmatched { - msg := linter.RuleUndeclaredArgInFrom.Format(unmatched) - linter.RuleUndeclaredArgInFrom.Run(opt.Warn, st.Location, msg) - } - } + nameMatch, err := shlex.ProcessWordWithMatches(st.BaseName, metaArgsToMap(optMetaArgs)) + reportUnusedFromArgs(nameMatch.Unmatched, st.Location, opt.Warn) + used := nameMatch.Matched + if err != nil { return nil, parser.WithLocation(err, st.Location) } - if name == "" { + if nameMatch.Result == "" { return nil, parser.WithLocation(errors.Errorf("base name (%s) should not be blank", st.BaseName), st.Location) } - st.BaseName = name + st.BaseName = nameMatch.Result ds := &dispatchState{ stage: st, @@ -289,27 +284,22 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS } if v := st.Platform; v != "" { - result, err := shlex.ProcessWordWithMatches(v, metaArgsToMap(optMetaArgs)) - v := result.Word - u := result.Matched - - if len(result.Unmatched) > 0 { - for unmatched := range result.Unmatched { - msg := linter.RuleUndeclaredArgInFrom.Format(unmatched) - linter.RuleUndeclaredArgInFrom.Run(opt.Warn, st.Location, msg) - } - } + platMatch, err := shlex.ProcessWordWithMatches(v, metaArgsToMap(optMetaArgs)) + reportUnusedFromArgs(platMatch.Unmatched, st.Location, opt.Warn) + if err != nil { - return nil, parser.WithLocation(errors.Wrapf(err, "failed to process arguments for platform %s", v), st.Location) + return nil, parser.WithLocation(errors.Wrapf(err, "failed to process arguments for platform %s", platMatch.Result), st.Location) } - p, err := platforms.Parse(v) + p, err := platforms.Parse(platMatch.Result) if err != nil { - return nil, parser.WithLocation(errors.Wrapf(err, "failed to parse platform %s", v), st.Location) + return nil, parser.WithLocation(errors.Wrapf(err, "failed to parse platform %s", platMatch.Result), st.Location) } - for k := range u { + + for k := range platMatch.Matched { used[k] = struct{}{} } + ds.platform = &p } @@ -2093,3 +2083,10 @@ func toPBLocation(sourceIndex int, location []parser.Range) pb.Location { Ranges: loc, } } + +func reportUnusedFromArgs(unmatched map[string]struct{}, location []parser.Range, warn linter.LintWarnFunc) { + for arg := range unmatched { + msg := linter.RuleUndeclaredArgInFrom.Format(arg) + linter.RuleUndeclaredArgInFrom.Run(warn, location, msg) + } +} diff --git a/frontend/dockerfile/dockerfile_lint_test.go b/frontend/dockerfile/dockerfile_lint_test.go index c65e028a5b149..34e2637436ec7 100644 --- a/frontend/dockerfile/dockerfile_lint_test.go +++ b/frontend/dockerfile/dockerfile_lint_test.go @@ -344,6 +344,13 @@ FROM ${BAR} AS base Line: 3, Level: 1, }, + { + RuleName: "UndeclaredArgInFrom", + Description: "FROM command must use declared ARGs", + Detail: "FROM argument 'BAR' is not declared", + Level: 1, + Line: 4, + }, }, StreamBuildErr: "failed to solve: base name (${BAR}) should not be blank", UnmarshalBuildErr: "base name (${BAR}) should not be blank", @@ -360,12 +367,38 @@ COPY Dockerfile . checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile}) dockerfile = []byte(` -ARG platform=linux/amd64 -FROM --platform=$platform scratch +ARG BUILDPLATFORM=linux/amd64 +FROM --platform=$BUILDPLATFORM scratch +COPY Dockerfile . +`) + checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile}) + + dockerfile = []byte(` +FROM --platform=$BUILDPLATFORM scratch COPY Dockerfile . `) checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile}) + dockerfile = []byte(` +FROM --platform=$BULIDPLATFORM scratch +COPY Dockerfile . +`) + checkLinterWarnings(t, sb, &lintTestParams{ + Dockerfile: dockerfile, + Warnings: []expectedLintWarning{ + { + RuleName: "UndeclaredArgInFrom", + Description: "FROM command must use declared ARGs", + Detail: "FROM argument 'BULIDPLATFORM' is not declared", + Level: 1, + Line: 2, + }, + }, + StreamBuildErr: "failed to solve: failed to parse platform : \"\" is an invalid component of \"\": platform specifier component must match \"^[A-Za-z0-9_-]+$\": invalid argument", + UnmarshalBuildErr: "failed to parse platform : \"\" is an invalid component of \"\": platform specifier component must match \"^[A-Za-z0-9_-]+$\": invalid argument", + BuildErrLocation: 2, + }) + dockerfile = []byte(` ARG tag=latest FROM busybox:${tag}${version} AS b @@ -434,7 +467,6 @@ func checkUnmarshal(t *testing.T, sb integration.Sandbox, lintTest *lintTestPara }, }, "", frontend, nil) require.NoError(t, err) - require.True(t, called) } @@ -503,6 +535,7 @@ func checkProgressStream(t *testing.T, sb integration.Sandbox, lintTest *lintTes } func checkLinterWarnings(t *testing.T, sb integration.Sandbox, lintTest *lintTestParams) { + t.Helper() sort.Slice(lintTest.Warnings, func(i, j int) bool { return lintTest.Warnings[i].Line < lintTest.Warnings[j].Line }) diff --git a/frontend/dockerfile/shell/lex.go b/frontend/dockerfile/shell/lex.go index c0a7bbfb98538..3ae23f5ea668c 100644 --- a/frontend/dockerfile/shell/lex.go +++ b/frontend/dockerfile/shell/lex.go @@ -58,7 +58,7 @@ func (s *Lex) ProcessWordWithMap(word string, env map[string]string) (string, er } type ProcessWordMatchesResult struct { - Word string + Result string Matched map[string]struct{} Unmatched map[string]struct{} } @@ -69,7 +69,7 @@ func (s *Lex) ProcessWordWithMatches(word string, env map[string]string) (Proces sw := s.init(word, env) word, _, err := sw.process(word) return ProcessWordMatchesResult{ - Word: word, + Result: word, Matched: sw.matches, Unmatched: sw.nonmatches, }, err diff --git a/frontend/dockerfile/shell/lex_test.go b/frontend/dockerfile/shell/lex_test.go index 288fcf2be697b..43d65afa5acb2 100644 --- a/frontend/dockerfile/shell/lex_test.go +++ b/frontend/dockerfile/shell/lex_test.go @@ -271,6 +271,7 @@ func TestProcessWithMatches(t *testing.T) { expected string expectedErr bool matches map[string]struct{} + unmatched map[string]struct{} }{ { input: "x", @@ -481,7 +482,7 @@ func TestProcessWithMatches(t *testing.T) { c := c t.Run(c.input, func(t *testing.T) { result, err := shlex.ProcessWordWithMatches(c.input, c.envs) - w := result.Word + w := result.Result matches := result.Matched if c.expectedErr { require.Error(t, err) @@ -514,7 +515,7 @@ func TestProcessWithMatchesPlatform(t *testing.T) { "TARGETVARIANT": "v7", }) require.NoError(t, err) - require.Equal(t, "something-v1.2.3.linux-arm-v7.tar.gz", results.Word) + require.Equal(t, "something-v1.2.3.linux-arm-v7.tar.gz", results.Result) results, err = shlex.ProcessWordWithMatches(release, map[string]string{ "VERSION": version, @@ -523,7 +524,7 @@ func TestProcessWithMatchesPlatform(t *testing.T) { "TARGETVARIANT": "", }) require.NoError(t, err) - require.Equal(t, "something-v1.2.3.linux-arm64.tar.gz", results.Word) + require.Equal(t, "something-v1.2.3.linux-arm64.tar.gz", results.Result) results, err = shlex.ProcessWordWithMatches(release, map[string]string{ "VERSION": version, @@ -532,5 +533,5 @@ func TestProcessWithMatchesPlatform(t *testing.T) { // No "TARGETVARIANT": "", }) require.NoError(t, err) - require.Equal(t, "something-v1.2.3.linux-arm64.tar.gz", results.Word) + require.Equal(t, "something-v1.2.3.linux-arm64.tar.gz", results.Result) }