fix, botreview: handle deleted files in kubevirtci-bump, improve commenting

external-plugins/botreview/review/BUILD.bazel

@@ -25,9 +25,15 @@ go_test(
         "bump_kubevirtci_test.go",
         "image_update_test.go",
         "prow_autobump_test.go",
+        "result_test.go",
         "review_test.go",
     ],
     data = glob(["testdata/**"]),
     embed = [":go_default_library"],
-    deps = ["@com_github_sourcegraph_go_diff//diff:go_default_library"],
+    deps = [
+        "@com_github_andreyvit_diff//:go_default_library",
+        "@com_github_sirupsen_logrus//:go_default_library",
+        "@com_github_sourcegraph_go_diff//diff:go_default_library",
+        "@io_k8s_test_infra//prow/github:go_default_library",
+    ],
 )

external-plugins/botreview/review/bump_kubevirtci.go

@@ -28,10 +28,7 @@ import (
 
 const (
 	bumpKubevirtCIApproveComment    = `:thumbsup: This looks like a simple kubevirtci bump.`
-	bumpKubevirtCIDisapproveComment = `:thumbsdown: This doesn't look like a simple kubevirtci bump.
-
-I found suspicious hunks:
-`
+	bumpKubevirtCIDisapproveComment = `:thumbsdown: This doesn't look like a simple kubevirtci bump.`
 )
 
 var bumpKubevirtCIHackConfigDefaultMatcher *regexp.Regexp
@@ -46,7 +43,7 @@ func init() {
 
 type BumpKubevirtCI struct {
 	relevantFileDiffs []*diff.FileDiff
-	unwantedFiles     map[string][]*diff.Hunk
+	unwantedFiles     map[string]struct{}
 }
 
 func (t *BumpKubevirtCI) IsRelevant() bool {
@@ -56,25 +53,23 @@ func (t *BumpKubevirtCI) IsRelevant() bool {
 func (t *BumpKubevirtCI) AddIfRelevant(fileDiff *diff.FileDiff) {
 	fileName := strings.TrimPrefix(fileDiff.NewName, "b/")
 
-	// store all hunks for unwanted files
-	if fileName != "cluster-up-sha.txt" &&
-		fileName != "hack/config-default.sh" &&
-		!strings.HasPrefix(fileName, "cluster-up/") {
-		for _, hunk := range fileDiff.Hunks {
-			if t.unwantedFiles == nil {
-				t.unwantedFiles = make(map[string][]*diff.Hunk, 0)
-			}
-			_, exists := t.unwantedFiles[fileName]
-			if !exists {
-				t.unwantedFiles[fileName] = []*diff.Hunk{hunk}
-			} else {
-				t.unwantedFiles[fileName] = append(t.unwantedFiles[fileName], hunk)
-			}
-		}
+	// handle deleted files
+	wasDeleted := fileName == "/dev/null"
+	if wasDeleted {
+		fileName = strings.TrimPrefix(fileDiff.OrigName, "a/")
+	}
+
+	if fileName == "cluster-up-sha.txt" ||
+		fileName == "hack/config-default.sh" ||
+		strings.HasPrefix(fileName, "cluster-up/") {
+		t.relevantFileDiffs = append(t.relevantFileDiffs, fileDiff)
 		return
 	}
 
-	t.relevantFileDiffs = append(t.relevantFileDiffs, fileDiff)
+	if t.unwantedFiles == nil {
+		t.unwantedFiles = make(map[string]struct{})
+	}
+	t.unwantedFiles[fileName] = struct{}{}
 }
 
 func (t *BumpKubevirtCI) Review() BotReviewResult {
@@ -103,8 +98,8 @@ func (t *BumpKubevirtCI) Review() BotReviewResult {
 		}
 	}
 
-	for fileName, unwantedFiles := range t.unwantedFiles {
-		result.AddReviewFailure(fileName, unwantedFiles...)
+	for fileName := range t.unwantedFiles {
+		result.AddReviewFailure(fileName)
 	}
 
 	return result

external-plugins/botreview/review/bump_kubevirtci_test.go

@@ -20,6 +20,7 @@
 package review
 
 import (
+	"fmt"
 	"github.com/sourcegraph/go-diff/diff"
 	"os"
 	"path/filepath"
@@ -28,7 +29,31 @@ import (
 )
 
 func TestBumpKubevirtCI_Review(t1 *testing.T) {
-	diffFilePaths := []string{}
+	diffFilePaths := []string{
+		"testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch00",
+		"testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch01",
+		"testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch02",
+		"testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch03",
+		"testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch04",
+		"testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch05",
+		"testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch06",
+		"testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch07",
+		"testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch08",
+		"testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch09",
+		"testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch10",
+		"testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch11",
+		"testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch12",
+		"testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch13",
+		"testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch14",
+		"testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch15",
+		"testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch16",
+		"testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch00",
+		"testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch01",
+		"testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch02",
+		"testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch03",
+		"testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch04",
+		"testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch05",
+	}
 	entries, err := os.ReadDir("testdata/kubevirtci-bump")
 	if err != nil {
 		t1.Errorf("failed to read files: %v", err)
@@ -47,6 +72,9 @@ func TestBumpKubevirtCI_Review(t1 *testing.T) {
 		if err != nil {
 			t1.Errorf("failed to read diff: %v", err)
 		}
+		if bumpFileDiffs == nil {
+			panic(fmt.Sprintf("file diff %q empty", diffFile))
+		}
 		diffFilePathsToDiffs[diffFile] = bumpFileDiffs
 	}
 	type fields struct {
@@ -58,7 +86,7 @@ func TestBumpKubevirtCI_Review(t1 *testing.T) {
 		want   BotReviewResult
 	}{
 		{
-			name: "simple prow autobump",
+			name: "simple kubevirtci-bump",
 			fields: fields{
 				relevantFileDiffs: []*diff.FileDiff{
 					diffFilePathsToDiffs["testdata/kubevirtci-bump/cluster-up-sha.txt"],
@@ -74,15 +102,72 @@ func TestBumpKubevirtCI_Review(t1 *testing.T) {
 			want: newReviewResultWithData(bumpKubevirtCIApproveComment, bumpKubevirtCIDisapproveComment, nil, ""),
 		},
 		{
-			name: "mixed image bump",
+			name: "mixed kubevirtci-bump",
 			fields: fields{
 				relevantFileDiffs: []*diff.FileDiff{
 					diffFilePathsToDiffs["testdata/kubevirtci-bump/cluster-up-sha.txt"],
 					diffFilePathsToDiffs["testdata/kubevirtci-bump/cluster-up_cluster_kind-1.22-sriov_provider.sh"],
 					diffFilePathsToDiffs["testdata/mixed_bump_prow_job.patch0"],
 				},
 			},
-			want: newReviewResultWithData(bumpKubevirtCIApproveComment, bumpKubevirtCIDisapproveComment, map[string][]*diff.Hunk{"github/ci/prow-deploy/files/jobs/kubevirt/kubevirt/kubevirt-presubmits.yaml": diffFilePathsToDiffs["testdata/mixed_bump_prow_job.patch0"].Hunks}, ""),
+			want: newReviewResultWithData(bumpKubevirtCIApproveComment, bumpKubevirtCIDisapproveComment, map[string][]*diff.Hunk{"github/ci/prow-deploy/files/jobs/kubevirt/kubevirt/kubevirt-presubmits.yaml": nil}, ""),
+		},
+		{
+			name: "non kubevirtci bump",
+			fields: fields{
+				relevantFileDiffs: []*diff.FileDiff{
+					diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch00"],
+					diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch01"],
+					diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch02"],
+					diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch03"],
+					diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch04"],
+					diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch05"],
+					diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch06"],
+					diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch07"],
+					diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch08"],
+					diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch09"],
+					diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch10"],
+					diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch11"],
+					diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch12"],
+					diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch13"],
+					diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch14"],
+					diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch15"],
+					diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch16"],
+				},
+			},
+			want: newReviewResultWithData(bumpKubevirtCIApproveComment, bumpKubevirtCIDisapproveComment, map[string][]*diff.Hunk{
+				"pkg/virt-operator/resource/generate/components/daemonsets.go":            nil,
+				"pkg/container-disk/container-disk_test.go":                               nil,
+				"pkg/virt-handler/container-disk/BUILD.bazel":                             nil,
+				"staging/src/kubevirt.io/api/core/v1/types_swagger_generated.go":          nil,
+				"staging/src/kubevirt.io/client-go/api/openapi_generated.go":              nil,
+				"staging/src/kubevirt.io/api/core/v1/types.go":                            nil,
+				"api/openapi-spec/swagger.json":                                           nil,
+				"pkg/virt-handler/container-disk/mount.go":                                nil,
+				"pkg/virt-controller/services/template.go":                                nil,
+				"staging/src/kubevirt.io/api/core/v1/deepcopy_generated.go":               nil,
+				"pkg/virt-operator/resource/generate/components/validations_generated.go": nil,
+				"pkg/virt-handler/vm.go":                                                  nil,
+				"pkg/virt-handler/container-disk/generated_mock_mount.go":                 nil,
+				"pkg/virt-handler/isolation/isolation.go":                                 nil,
+				"pkg/virt-handler/container-disk/mount_test.go":                           nil,
+				"pkg/virt-handler/vm_test.go":                                             nil,
+				"pkg/container-disk/container-disk.go":                                    nil,
+			}, ""),
+		},
+		{
+			name: "kubevirtci-bump with deleted files",
+			fields: fields{
+				relevantFileDiffs: []*diff.FileDiff{
+					diffFilePathsToDiffs["testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch00"],
+					diffFilePathsToDiffs["testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch01"],
+					diffFilePathsToDiffs["testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch02"],
+					diffFilePathsToDiffs["testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch03"],
+					diffFilePathsToDiffs["testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch04"],
+					diffFilePathsToDiffs["testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch05"],
+				},
+			},
+			want: newReviewResultWithData(bumpKubevirtCIApproveComment, bumpKubevirtCIDisapproveComment, nil, ""),
 		},
 	}
 	for _, tt := range tests {

external-plugins/botreview/review/image_update.go

@@ -28,15 +28,12 @@ import (
 
 const (
 	prowJobImageUpdateApproveComment    = `:thumbsup: This looks like a simple prow job image bump.`
-	prowJobImageUpdateDisapproveComment = `:thumbsdown: This doesn't look like a simple prow job image bump.
-
-I found suspicious hunks:
-`
+	prowJobImageUpdateDisapproveComment = `:thumbsdown: This doesn't look like a simple prow job image bump.`
 )
 
 var (
 	prowJobImageUpdateLineMatcher       = regexp.MustCompile(`(?m)^\+\s+- image: \S+$`)
-	prowJobImageUpdateHunkBodyMatcher   = regexp.MustCompile(`(?m)^(-\s+- image: \S+$\n^\+\s+- image: \S+|-\s+image: \S+$\n^\+\s+image: \S+)$`)
+	prowJobImageUpdateHunkBodyMatcher   = regexp.MustCompile(`(?m)^(-\s+- image:\s+\S+$\n^\+\s+- image:\s+\S+|-\s+image:\s+\S+$\n^\+\s+image:\s+\S+)$`)
 	prowJobReleaseBranchFileNameMatcher = regexp.MustCompile(`.*/[\w-]+-[0-9-.]+\.yaml`)
 )
 
@@ -86,7 +83,7 @@ func (t *ProwJobImageUpdate) Review() BotReviewResult {
 	for _, fileDiff := range t.relevantFileDiffs {
 		fileName := strings.TrimPrefix(fileDiff.NewName, "b/")
 		for _, hunk := range fileDiff.Hunks {
-			if !prowJobImageUpdateHunkBodyMatcher.Match(hunk.Body) {
+			if !t.hunkMatches(hunk) {
 				result.AddReviewFailure(fileName, hunk)
 			}
 		}
@@ -95,6 +92,10 @@ func (t *ProwJobImageUpdate) Review() BotReviewResult {
 	return result
 }
 
+func (t *ProwJobImageUpdate) hunkMatches(hunk *diff.Hunk) bool {
+	return prowJobImageUpdateHunkBodyMatcher.Match(hunk.Body)
+}
+
 func (t *ProwJobImageUpdate) String() string {
 	return fmt.Sprintf("relevantFileDiffs: %v", t.relevantFileDiffs)
 }

external-plugins/botreview/review/image_update_test.go

@@ -161,3 +161,105 @@ func TestProwJobImageUpdate_AddIfRelevant(t1 *testing.T) {
 		})
 	}
 }
+
+func TestProwJobImageUpdate_hunkMatches(t1 *testing.T) {
+	type fields struct {
+		relevantFileDiffs []*diff.FileDiff
+		notMatchingHunks  []*diff.Hunk
+	}
+	type args struct {
+		hunk *diff.Hunk
+	}
+	tests := []struct {
+		name   string
+		fields fields
+		args   args
+		want   bool
+	}{
+		{
+			name:   "image update 1",
+			fields: fields{},
+			args: args{
+				hunk: &diff.Hunk{
+					Body: []byte(`       nodeSelector:
+         type: bare-metal-external
+       containers:
+-        - image:  quay.io/kubevirtci/bootstrap:v20231115-51a244f
++        - image:  quay.io/kubevirtci/bootstrap:v20231219-bf5e580
+           command:
+             - "/usr/local/bin/runner.sh"
+             - "/bin/sh"
+`),
+				},
+			},
+			want: true,
+		},
+		{
+			name:   "image update 2",
+			fields: fields{},
+			args: args{
+				hunk: &diff.Hunk{
+					Body: []byte(`       cluster: prow-workloads
+       spec:
+         containers:
+-          - image: quay.io/kubevirtci/bootstrap:v20201119-a5880e0
++          - image: quay.io/kubevirtci/bootstrap:v20220110-c066ff5
+             command:
+               - "/usr/local/bin/runner.sh"
+               - "/bin/bash"
+`),
+				},
+			},
+			want: true,
+		},
+		{
+			name:   "image update 3",
+			fields: fields{},
+			args: args{
+				hunk: &diff.Hunk{
+					Body: []byte(`         nodeSelector:
+           type: bare-metal-external
+         containers:
+-          - image: quay.io/kubevirtci/kubevirt-infra-bootstrap:v20201201-08dc4a9
++          - image: quay.io/kubevirtci/kubevirt-infra-bootstrap:v20210419-444033d
+             securityContext:
+               privileged: true
+             resources:
+`),
+				},
+			},
+			want: true,
+		},
+		{
+			name:   "mixed update 1",
+			fields: fields{},
+			args: args{
+				hunk: &diff.Hunk{
+					Body: []byte(`         - automation/test.sh
+         env:
+         - name: TARGET
+-          value: k8s-1.21-sig-network
+-        image: quay.io/kubevirtci/bootstrap:v20220602-ac34bf7
++          value: k8s-1.21-sig-storage
++        image: quay.io/kubevirtci/bootstrap:v20999999-eeff777
+         name: ""
+         resources:
+           requests:
+`),
+				},
+			},
+			want: false,
+		},
+	}
+	for _, tt := range tests {
+		t1.Run(tt.name, func(t1 *testing.T) {
+			t := &ProwJobImageUpdate{
+				relevantFileDiffs: tt.fields.relevantFileDiffs,
+				notMatchingHunks:  tt.fields.notMatchingHunks,
+			}
+			if got := t.hunkMatches(tt.args.hunk); got != tt.want {
+				t1.Errorf("hunkMatches() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}

external-plugins/botreview/review/prow_autobump.go

@@ -27,11 +27,8 @@ import (
 )
 
 const (
-	prowAutobumpApproveComment    = `:thumbsup: This looks like a simple prow autobump.`
-	prowAutobumpDisapproveComment = `:thumbsdown: This doesn't look like a simple prow autobump.
-
-I found suspicious hunks:
-`
+	prowAutobumpApproveComment       = `:thumbsup: This looks like a simple prow autobump.`
+	prowAutobumpDisapproveComment    = `:thumbsdown: This doesn't look like a simple prow autobump.`
 	prowAutoBumpShouldNotMergeReason = "prow update should be merged at a point in time where it doesn't interfere with normal CI usage"
 )