From f93ea5cf3ca956943711bbf0d4d86e330f248534 Mon Sep 17 00:00:00 2001 From: xingweizheng 00591739 Date: Mon, 30 Nov 2020 17:13:15 +0800 Subject: [PATCH 4/4] fix printing FROM command double times to console --- builder/dockerfile/builder_test.go | 16 ++--- builder/dockerfile/cmd_builder_test.go | 41 ++++++------- builder/dockerfile/parser/parser.go | 12 ++-- builder/dockerfile/parser/parser_test.go | 61 ++++++++++++++++++- .../preprocess/busybox_line_with_spaces | 0 .../testfiles/preprocess/busybox_no_command | 2 +- .../preprocess/busybox_ubuntu_centos | 3 + .../preprocess/busybox_with_from_only | 1 + .../compelte_stage_with_single_from_stage | 3 + ...single_from_stage_depend_on_previous_stage | 3 + .../final_stage_depend_on_previous_stage | 3 + .../single_from_stage_with_complete_stage | 3 + builder/dockerfile/stage_builder.go | 12 +++- 13 files changed, 122 insertions(+), 38 deletions(-) mode change 100755 => 100644 builder/dockerfile/parser/testfiles/preprocess/busybox_line_with_spaces create mode 100755 builder/dockerfile/parser/testfiles/preprocess/busybox_ubuntu_centos create mode 100755 builder/dockerfile/parser/testfiles/preprocess/busybox_with_from_only create mode 100755 builder/dockerfile/parser/testfiles/preprocess/compelte_stage_with_single_from_stage create mode 100755 builder/dockerfile/parser/testfiles/preprocess/final_single_from_stage_depend_on_previous_stage create mode 100755 builder/dockerfile/parser/testfiles/preprocess/final_stage_depend_on_previous_stage create mode 100755 builder/dockerfile/parser/testfiles/preprocess/single_from_stage_with_complete_stage diff --git a/builder/dockerfile/builder_test.go b/builder/dockerfile/builder_test.go index b02768ea..94842c71 100644 --- a/builder/dockerfile/builder_test.go +++ b/builder/dockerfile/builder_test.go @@ -248,13 +248,13 @@ RUN ls } // check the arg and env taken by the command: RUN ls - assert.DeepEqual(t, b.stageBuilders[0].commands[1].args, + assert.DeepEqual(t, b.stageBuilders[0].commands[0].args, map[string]string{"no_proxy": "10.0.0.0"}) - assert.DeepEqual(t, b.stageBuilders[1].commands[2].args, + assert.DeepEqual(t, b.stageBuilders[1].commands[1].args, map[string]string{"testArg": "0.1", "no_proxy": "10.0.0.0"}) - assert.DeepEqual(t, b.stageBuilders[2].commands[3].args, + assert.DeepEqual(t, b.stageBuilders[2].commands[2].args, map[string]string{"no_proxy": "10.0.0.0"}) - assert.DeepEqual(t, b.stageBuilders[2].commands[3].envs, + assert.DeepEqual(t, b.stageBuilders[2].commands[2].envs, map[string]string{"testArg": "1.0"}) } @@ -291,13 +291,13 @@ RUN ls } // check the arg and env taken by the command: RUN ls - assert.DeepEqual(t, b.stageBuilders[0].commands[1].args, + assert.DeepEqual(t, b.stageBuilders[0].commands[0].args, map[string]string{"HTTPS_PROXY": "127.0.0.1"}) - assert.DeepEqual(t, b.stageBuilders[1].commands[2].args, + assert.DeepEqual(t, b.stageBuilders[1].commands[1].args, map[string]string{"testArg": "0.1", "HTTPS_PROXY": "127.0.0.1"}) - assert.DeepEqual(t, b.stageBuilders[2].commands[3].args, + assert.DeepEqual(t, b.stageBuilders[2].commands[2].args, map[string]string{"HTTPS_PROXY": "127.0.0.1"}) - assert.DeepEqual(t, b.stageBuilders[2].commands[3].envs, + assert.DeepEqual(t, b.stageBuilders[2].commands[2].envs, map[string]string{"testArg": "1.0"}) } diff --git a/builder/dockerfile/cmd_builder_test.go b/builder/dockerfile/cmd_builder_test.go index df1c08f9..471314ba 100644 --- a/builder/dockerfile/cmd_builder_test.go +++ b/builder/dockerfile/cmd_builder_test.go @@ -113,7 +113,7 @@ func TestExecuteHealthCheck(t *testing.T) { } err := s.analyzeStage(context.Background()) assert.NilError(t, err) - if err := s.commands[1].cmdExecutor(); (err != nil) != tt.wantErr { + if err := s.commands[0].cmdExecutor(); (err != nil) != tt.wantErr { t.Errorf("CmdExecutor() error: %v, wantErr: %v", err, tt.wantErr) } tt.funcCheck(t, s) @@ -195,7 +195,7 @@ CMD [""]`, } err := s.analyzeStage(context.Background()) assert.NilError(t, err) - if err := s.commands[1].cmdExecutor(); (err != nil) != tt.wantErr { + if err := s.commands[0].cmdExecutor(); (err != nil) != tt.wantErr { t.Errorf("cmdExecutor() error: %v, wantErr: %v", err, tt.wantErr) } tt.funcCheck(t, s) @@ -288,7 +288,7 @@ SHELL ["/bin/bash", "-c"]`, } err := s.analyzeStage(context.Background()) assert.NilError(t, err) - if err := s.commands[1].cmdExecutor(); (err != nil) != tt.wantErr { + if err := s.commands[0].cmdExecutor(); (err != nil) != tt.wantErr { t.Errorf("SHELL cmdExecutor() error: %v, wantErr: %v", err, tt.wantErr) } tt.funcCheck(t, s) @@ -316,15 +316,15 @@ CMD ls` err := s.analyzeStage(context.Background()) assert.NilError(t, err) - if err := s.commands[1].cmdExecutor(); err != nil { + if err := s.commands[0].cmdExecutor(); err != nil { t.Errorf("CMD cmdExecutor() error: %v", err) } assert.DeepEqual(t, s.docker.Config.Cmd, strslice.StrSlice{"/bin/sh", "-c", "ls"}) - if err := s.commands[2].cmdExecutor(); err != nil { + if err := s.commands[1].cmdExecutor(); err != nil { t.Errorf("SHELL cmdExecutor() error: %v", err) } - if err := s.commands[3].cmdExecutor(); err != nil { + if err := s.commands[2].cmdExecutor(); err != nil { t.Errorf("CMD cmdExecutor() error: %v", err) } assert.DeepEqual(t, s.shellForm, strslice.StrSlice{"/bin/bash", "-c"}) @@ -360,10 +360,9 @@ func TestExecuteNoop(t *testing.T) { // the "STEP 1: FROM alpine" in production is done at stageBuilder.prepare() // no cmdExecutor for FROM, so no print for FROM here - expectedString := `STEP 1: FROM alpine -STEP 2: ARG testArg -STEP 3: ENV env1=env2 -STEP 4: ONBUILD CMD ls + expectedString := `STEP 1: ARG testArg +STEP 2: ENV env1=env2 +STEP 3: ONBUILD CMD ls ` assert.Equal(t, stepPrints, expectedString) } @@ -442,7 +441,7 @@ ENTRYPOINT [""]`, } err := s.analyzeStage(context.Background()) assert.NilError(t, err) - if err := s.commands[1].cmdExecutor(); (err != nil) != tt.wantErr { + if err := s.commands[0].cmdExecutor(); (err != nil) != tt.wantErr { t.Errorf("cmdExecutor() error: %v, wantErr: %v", err, tt.wantErr) } tt.funcCheck(t, s) @@ -904,15 +903,15 @@ WORKDIR /c` } err := s.analyzeStage(context.Background()) assert.NilError(t, err) - if err := s.commands[1].cmdExecutor(); err != nil { + if err := s.commands[0].cmdExecutor(); err != nil { t.Errorf("WORKDIR cmdExecutor() error: %v", err) } assert.DeepEqual(t, s.docker.Config.WorkingDir, "/a") - if err := s.commands[2].cmdExecutor(); err != nil { + if err := s.commands[1].cmdExecutor(); err != nil { t.Errorf("WORKDIR cmdExecutor() error: %v", err) } assert.DeepEqual(t, s.docker.Config.WorkingDir, "/b") - if err := s.commands[3].cmdExecutor(); err != nil { + if err := s.commands[2].cmdExecutor(); err != nil { t.Errorf("WORKDIR cmdExecutor() error: %v", err) } assert.DeepEqual(t, s.docker.Config.WorkingDir, "/c") @@ -939,15 +938,15 @@ WORKDIR c` } err := s.analyzeStage(context.Background()) assert.NilError(t, err) - if err := s.commands[1].cmdExecutor(); err != nil { + if err := s.commands[0].cmdExecutor(); err != nil { t.Errorf("WORKDIR cmdExecutor() error: %v", err) } assert.DeepEqual(t, s.docker.Config.WorkingDir, "/a") - if err := s.commands[2].cmdExecutor(); err != nil { + if err := s.commands[1].cmdExecutor(); err != nil { t.Errorf("WORKDIR cmdExecutor() error: %v", err) } assert.DeepEqual(t, s.docker.Config.WorkingDir, "/a/b") - if err := s.commands[3].cmdExecutor(); err != nil { + if err := s.commands[2].cmdExecutor(); err != nil { t.Errorf("WORKDIR cmdExecutor() error: %v", err) } assert.DeepEqual(t, s.docker.Config.WorkingDir, "/a/b/c") @@ -974,15 +973,15 @@ WORKDIR $DIRPATH/$DIRNAME` } err := s.analyzeStage(context.Background()) assert.NilError(t, err) - if err = s.commands[1].cmdExecutor(); err != nil { + if err = s.commands[0].cmdExecutor(); err != nil { t.Errorf("WORKDIR cmdExecutor() error: %v", err) } assert.NilError(t, err) - if err = s.commands[2].cmdExecutor(); err != nil { + if err = s.commands[1].cmdExecutor(); err != nil { t.Errorf("WORKDIR cmdExecutor() error: %v", err) } assert.NilError(t, err) - if err = s.commands[3].cmdExecutor(); err != nil { + if err = s.commands[2].cmdExecutor(); err != nil { t.Errorf("WORKDIR cmdExecutor() error: %v", err) } assert.NilError(t, err) @@ -1026,7 +1025,7 @@ Maintainer iSula iSula@huawei.com`, } err := s.analyzeStage(context.Background()) assert.NilError(t, err) - if err := s.commands[1].cmdExecutor(); (err != nil) != tt.wantErr { + if err := s.commands[0].cmdExecutor(); (err != nil) != tt.wantErr { t.Errorf("cmdExecutor() error: %v, wantErr: %v", err, tt.wantErr) } tt.funcCheck(t, s) diff --git a/builder/dockerfile/parser/parser.go b/builder/dockerfile/parser/parser.go index 82835e67..821e18ea 100644 --- a/builder/dockerfile/parser/parser.go +++ b/builder/dockerfile/parser/parser.go @@ -238,19 +238,21 @@ func constructPages(lines []*parser.Line, onbuild bool) ([]*parser.Page, error) } pageMap[page.Name] = page // if the base image for current stage is from the previous stage, - // mark the previous stage need to commit - if from, ok := pageMap[line.Cells[0].Value]; ok { + // mark the previous stage need to commit, for only from command we don't commit + if from, ok := pageMap[line.Cells[0].Value]; ok && len(from.Lines) > 1 { from.NeedCommit = true } currentPage = page } // because a valid dockerfile is always start with 'FROM' command here, so no need - // to check currentPage wheather is nil + // to check whether currentPage is nil or not currentPage.End = line.End currentPage.AddLine(line) } - // the last stage always need to commit - currentPage.NeedCommit = true + // the last stage always need to commit except page that contains only from command + if len(currentPage.Lines) > 1 { + currentPage.NeedCommit = true + } pages = append(pages, currentPage) if len(pages) == 0 { diff --git a/builder/dockerfile/parser/parser_test.go b/builder/dockerfile/parser/parser_test.go index fe27dd95..8580b84c 100644 --- a/builder/dockerfile/parser/parser_test.go +++ b/builder/dockerfile/parser/parser_test.go @@ -119,7 +119,6 @@ func TestFormatWithSpacesAfterEscapeToken(t *testing.T) { name: "busybox_line_with_spaces", expect: []int{12, 20, 96, 87, 10}, }, - } for _, tc := range testcases { @@ -194,8 +193,10 @@ func TestParse(t *testing.T) { r, err := os.Open(file) assert.NilError(t, err) defer r.Close() + df := dockerfile{} _, err = df.Parse(r, false) + if !tc.isErr { assert.NilError(t, err, file) } else { @@ -205,6 +206,64 @@ func TestParse(t *testing.T) { } } +func TestParseContainSingleFrom(t *testing.T) { + testcases := []struct { + name string + isErr bool + committed bool + }{ + { + name: "busybox_with_from_only", + isErr: false, + committed: false, + }, { + name: "busybox_ubuntu_centos", + isErr: false, + committed: false, + }, { + name: "compelte_stage_with_single_from_stage", + isErr: false, + committed: false, + }, { + name: "single_from_stage_with_complete_stage", + isErr: false, + committed: true, + }, { + name: "final_single_from_stage_depend_on_previous_stage", + isErr: false, + committed: true, + }, { + name: "final_stage_depend_on_previous_stage", + isErr: false, + committed: true, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + file := filepath.Join("testfiles", "preprocess", tc.name) + r, err := os.Open(file) + assert.NilError(t, err) + defer r.Close() + + df := dockerfile{} + playbook := &parser.PlayBook{} + playbook, err = df.Parse(r, false) + + if !tc.isErr { + assert.NilError(t, err, file) + if tc.committed { + needCommit := false + for _, page := range playbook.Pages { + needCommit = page.NeedCommit || needCommit + } + assert.Equal(t, needCommit, true) + } + } + }) + } +} + func TestParseIgnore(t *testing.T) { dockerignore := ` # comment diff --git a/builder/dockerfile/parser/testfiles/preprocess/busybox_line_with_spaces b/builder/dockerfile/parser/testfiles/preprocess/busybox_line_with_spaces old mode 100755 new mode 100644 diff --git a/builder/dockerfile/parser/testfiles/preprocess/busybox_no_command b/builder/dockerfile/parser/testfiles/preprocess/busybox_no_command index da465805..a424e7e0 100644 --- a/builder/dockerfile/parser/testfiles/preprocess/busybox_no_command +++ b/builder/dockerfile/parser/testfiles/preprocess/busybox_no_command @@ -6,5 +6,5 @@ ENTRYPOINT ["sh"] RUN ["ls"] RUN echo "hello world" -# fail at here +# Support single FROM command at here FROM busybox diff --git a/builder/dockerfile/parser/testfiles/preprocess/busybox_ubuntu_centos b/builder/dockerfile/parser/testfiles/preprocess/busybox_ubuntu_centos new file mode 100755 index 00000000..57f870ad --- /dev/null +++ b/builder/dockerfile/parser/testfiles/preprocess/busybox_ubuntu_centos @@ -0,0 +1,3 @@ +FROM busybox +FROM ubuntu +FROM centos \ No newline at end of file diff --git a/builder/dockerfile/parser/testfiles/preprocess/busybox_with_from_only b/builder/dockerfile/parser/testfiles/preprocess/busybox_with_from_only new file mode 100755 index 00000000..84662517 --- /dev/null +++ b/builder/dockerfile/parser/testfiles/preprocess/busybox_with_from_only @@ -0,0 +1 @@ +FROM busybox \ No newline at end of file diff --git a/builder/dockerfile/parser/testfiles/preprocess/compelte_stage_with_single_from_stage b/builder/dockerfile/parser/testfiles/preprocess/compelte_stage_with_single_from_stage new file mode 100755 index 00000000..636a75df --- /dev/null +++ b/builder/dockerfile/parser/testfiles/preprocess/compelte_stage_with_single_from_stage @@ -0,0 +1,3 @@ +FROM busybox +RUN touch /tmp/a_test.txt +FROM ubuntu \ No newline at end of file diff --git a/builder/dockerfile/parser/testfiles/preprocess/final_single_from_stage_depend_on_previous_stage b/builder/dockerfile/parser/testfiles/preprocess/final_single_from_stage_depend_on_previous_stage new file mode 100755 index 00000000..9493cbc0 --- /dev/null +++ b/builder/dockerfile/parser/testfiles/preprocess/final_single_from_stage_depend_on_previous_stage @@ -0,0 +1,3 @@ +FROM busybox as new_busybox +RUN touch /tmp/a_test.txt +FROM new_busybox \ No newline at end of file diff --git a/builder/dockerfile/parser/testfiles/preprocess/final_stage_depend_on_previous_stage b/builder/dockerfile/parser/testfiles/preprocess/final_stage_depend_on_previous_stage new file mode 100755 index 00000000..43bde170 --- /dev/null +++ b/builder/dockerfile/parser/testfiles/preprocess/final_stage_depend_on_previous_stage @@ -0,0 +1,3 @@ +FROM busybox as new_busybox +FROM new_busybox +RUN touch /tmp/a_test.txt \ No newline at end of file diff --git a/builder/dockerfile/parser/testfiles/preprocess/single_from_stage_with_complete_stage b/builder/dockerfile/parser/testfiles/preprocess/single_from_stage_with_complete_stage new file mode 100755 index 00000000..b4cf1190 --- /dev/null +++ b/builder/dockerfile/parser/testfiles/preprocess/single_from_stage_with_complete_stage @@ -0,0 +1,3 @@ +FROM ubuntu +FROM busybox +RUN touch /tmp/a_test.txt \ No newline at end of file diff --git a/builder/dockerfile/stage_builder.go b/builder/dockerfile/stage_builder.go index 47f55bf1..23f488cb 100644 --- a/builder/dockerfile/stage_builder.go +++ b/builder/dockerfile/stage_builder.go @@ -172,7 +172,9 @@ func (s *stageBuilder) analyzeStage(ctx context.Context) error { cb := newCmdBuilder(ctx, line, s, stageArgs, stageEnvs) switch line.Command { + // From cmd is already pre-processed, we just pass it case dockerfile.From: + continue case dockerfile.Arg: if cb.args, err = analyzeArg(s.builder, line, stageArgs, stageEnvs); err != nil { return err @@ -209,10 +211,16 @@ func (s *stageBuilder) stageBuild(ctx context.Context) (string, error) { // 3. commit for new image if needed if s.rawStage.NeedCommit { - s.imageID, err = s.commit(ctx) + if s.imageID, err = s.commit(ctx); err != nil { + return s.imageID, errors.Wrapf(err, "commit image for stage %s failed", s.name) + } + } + // for only from command in Dockerfile, there is no imageID committed, use fromImageID + if s.imageID == "" { + s.imageID = s.fromImageID } - return s.imageID, errors.Wrapf(err, "commit image for stage %s failed", s.name) + return s.imageID, nil } func prepareImage(opt *image.PrepareImageOptions) (*image.Describe, error) { -- 2.27.0