Skip to content

Commit

Permalink
botreview, strategy: add kubevirt uploader (#3131)
Browse files Browse the repository at this point in the history 
* Add kubevirt uploader testdata

Signed-off-by: Daniel Hiller <dhiller@redhat.com>

* botreview, strategy: add kubevirt uploader

Signed-off-by: Daniel Hiller <dhiller@redhat.com>

* fix, kubevirt-upload: fix mixed upload test

Signed-off-by: Daniel Hiller <dhiller@redhat.com>

* fix, botreview: clean up details section

Signed-off-by: Daniel Hiller <dhiller@redhat.com>

* fix, botreview, kubevirt-upload: fix matching

Signed-off-by: Daniel Hiller <dhiller@redhat.com>

---------

Signed-off-by: Daniel Hiller <dhiller@redhat.com>
  • Loading branch information
@dhiller
dhiller authored Jan 13, 2024
1 parent 91f587e commit 080d85d
Show file tree
Hide file tree
Showing 7 changed files with 721 additions and 3 deletions.
2 changes: 2 additions & 0 deletions external-plugins/botreview/review/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ go_library(
srcs = [
"bump_kubevirtci.go",
"image_update.go",
"kubevirt_uploader.go",
"prow_autobump.go",
"result.go",
"review.go",
Expand All @@ -24,6 +25,7 @@ go_test(
srcs = [
"bump_kubevirtci_test.go",
"image_update_test.go",
"kubevirt_uploader_test.go",
"prow_autobump_test.go",
"result_test.go",
"review_test.go",
Expand Down
94 changes: 94 additions & 0 deletions external-plugins/botreview/review/kubevirt_uploader.go
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
/*
* This file is part of the KubeVirt project
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
* Copyright the KubeVirt authors.
*
*/

package review

import (
"fmt"
"github.com/sourcegraph/go-diff/diff"
"regexp"
"strings"
)

const (
kubevirtUploaderApproveComment = `:thumbsup: This looks like a simple kubevirt uploader bump.`
kubevirtUploaderDisapproveComment = `:thumbsdown: This doesn't look like a kubevirt uploader bump.`
)

var kubevirtUploaderMatcher *regexp.Regexp

func init() {
kubevirtUploaderMatcher = regexp.MustCompile(`(?m)^\+\s+"https://storage.googleapis.com/builddeps/\S+$`)
}

type KubeVirtUploader struct {
relevantFileDiffs []*diff.FileDiff
unwantedFiles map[string]struct{}
}

func (t *KubeVirtUploader) IsRelevant() bool {
return len(t.relevantFileDiffs) > 0
}

func (t *KubeVirtUploader) AddIfRelevant(fileDiff *diff.FileDiff) {
fileName := strings.TrimPrefix(fileDiff.NewName, "b/")

if fileName == "WORKSPACE" {
t.relevantFileDiffs = append(t.relevantFileDiffs, fileDiff)
return
}

if t.unwantedFiles == nil {
t.unwantedFiles = make(map[string]struct{})
}
t.unwantedFiles[fileName] = struct{}{}
}

func (t *KubeVirtUploader) Review() BotReviewResult {
result := NewCanMergeReviewResult(kubevirtUploaderApproveComment, kubevirtUploaderDisapproveComment)

for _, fileDiff := range t.relevantFileDiffs {
fileName := strings.TrimPrefix(fileDiff.NewName, "b/")
switch fileName {
case "WORKSPACE":
for _, hunk := range fileDiff.Hunks {
if !matchesKubeVirtUploaderPattern(hunk) {
result.AddReviewFailure(fileDiff.NewName, hunk)
}
}
default:
// no checks since we can't do anything reasonable here
continue
}
}

for fileName := range t.unwantedFiles {
result.AddReviewFailure(fileName)
}

return result
}

func matchesKubeVirtUploaderPattern(hunk *diff.Hunk) bool {
return kubevirtUploaderMatcher.Match(hunk.Body)
}

func (t *KubeVirtUploader) String() string {
return fmt.Sprintf("relevantFileDiffs: %v", t.relevantFileDiffs)
}
194 changes: 194 additions & 0 deletions external-plugins/botreview/review/kubevirt_uploader_test.go
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,194 @@
/*
* This file is part of the KubeVirt project
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
* Copyright the KubeVirt authors.
*
*/

package review

import (
"fmt"
"github.com/sourcegraph/go-diff/diff"
"os"
"reflect"
"testing"
)

func TestKubeVirtUploader_Review(t1 *testing.T) {
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",
"testdata/kubevirt/uploader/uploader-autoupdate.patch00",
}
diffFilePathsToDiffs := map[string]*diff.FileDiff{}
for _, diffFile := range diffFilePaths {
bumpImagesDiffFile, err := os.ReadFile(diffFile)
if err != nil {
t1.Errorf("failed to read diff: %v", err)
}
bumpFileDiffs, err := diff.ParseFileDiff(bumpImagesDiffFile)
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 {
relevantFileDiffs []*diff.FileDiff
}
tests := []struct {
name string
fields fields
want BotReviewResult
}{
{
name: "simple kubevirt upload",
fields: fields{
relevantFileDiffs: []*diff.FileDiff{
diffFilePathsToDiffs["testdata/kubevirt/uploader/uploader-autoupdate.patch00"],
},
},
want: newReviewResultWithData(kubevirtUploaderApproveComment, kubevirtUploaderDisapproveComment, nil, ""),
},
{
name: "mixed kubevirt upload",
fields: fields{
relevantFileDiffs: []*diff.FileDiff{
diffFilePathsToDiffs["testdata/kubevirt/uploader/uploader-autoupdate.patch00"],
diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch00"],
},
},
want: newReviewResultWithData(kubevirtUploaderApproveComment, kubevirtUploaderDisapproveComment, map[string][]*diff.Hunk{
"api/openapi-spec/swagger.json": nil,
}, ""),
},
{
name: "non kubevirt upload",
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(kubevirtUploaderApproveComment, kubevirtUploaderDisapproveComment, 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,
}, ""),
},
}
for _, tt := range tests {
t1.Run(tt.name, func(t1 *testing.T) {
t := &KubeVirtUploader{}
for index, fileDiff := range tt.fields.relevantFileDiffs {
if fileDiff == nil {
t1.Errorf("fileDiff[%d] nil", index)
}
t.AddIfRelevant(fileDiff)
}
if got := t.Review(); !reflect.DeepEqual(got, tt.want) {
t1.Errorf("Review() = %v, want %v", got, tt.want)
}
})
}
}

func Test_matchesKubeVirtUploaderPattern(t *testing.T) {
type args struct {
hunk *diff.Hunk
}
tests := []struct {
name string
args args
want bool
}{
{
name: "builddeps 1",
args: args{
hunk: &diff.Hunk{
Body: []byte(` urls = [
"https://mirror.bazel.build/github.com/bazelbuild/rules_go/releases/download/v0.42.0/rules_go-v0.42.0.zip",
"https://github.com/bazelbuild/rules_go/releases/download/v0.42.0/rules_go-v0.42.0.zip",
+ "https://storage.googleapis.com/builddeps/91585017debb61982f7054c9688857a2ad1fd823fc3f9cb05048b0025c47d023",
],
)
`),
},
},
want: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := matchesKubeVirtUploaderPattern(tt.args.hunk); got != tt.want {
t.Errorf("matchesKubeVirtUploaderPattern() = %v, want %v", got, tt.want)
}
})
}
}
6 changes: 3 additions & 3 deletions external-plugins/botreview/review/result.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,14 +77,14 @@ func (r *BasicReviewResult) String() string {
return r.approveComment
} else {
comment := r.disapproveComment
comment += fmt.Sprintf("\n\n<details>\n")
for fileName, hunks := range r.notMatchingHunks {
comment += fmt.Sprintf("\n\n<details>")
comment += fmt.Sprintf("\n\n_%s_", fileName)
comment += fmt.Sprintf("\n_%s_", fileName)
for _, hunk := range hunks {
comment += fmt.Sprintf("\n\n~~~diff\n%s\n~~~", string(hunk.Body))
}
comment += fmt.Sprintf("\n\n</details>\n")
}
comment += fmt.Sprintf("\n\n</details>\n")
return comment
}
}
Expand Down
1 change: 1 addition & 0 deletions external-plugins/botreview/review/review.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ func newPossibleReviewTypes() []KindOfChange {
&ProwJobImageUpdate{},
&BumpKubevirtCI{},
&ProwAutobump{},
&KubeVirtUploader{},
}
}

Expand Down
47 changes: 47 additions & 0 deletions external-plugins/botreview/review/review_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,53 @@ nil
</details>
This PR does not satisfy at least one automated review criteria.
Holding this PR because:
* should not get merged at all reason
/hold
**Note: botreview (kubevirt/project-infra#3100) is a Work In Progress!**
`,
},
},
},
{
name: "one review without hunks, not approved",
fields: newFields(),
args: args{
githubClient: newGHReviewClient(),
botReviewResults: []BotReviewResult{
newReviewResultWithData(
"approved",
"disapproved",
map[string][]*diff.Hunk{
"test": nil,
"blah": nil,
},
"should not get merged at all reason",
),
},
},
wantErr: false,
wantReviewComments: []*FakeComment{
{
Org: "",
Repo: "",
Number: 0,
Comment: `@pr-reviewer's review-bot says:
disapproved
<details>
_test_
_blah_
</details>
This PR does not satisfy at least one automated review criteria.
Holding this PR because:
Expand Down
Loading

0 comments on commit 080d85d

Please sign in to comment.