From f93ea5cf3ca956943711bbf0d4d86e330f248534 Mon Sep 17 00:00:00 2001
From: xingweizheng 00591739 <xingweizheng@huawei.com>
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

