Skip to content

Commit

Permalink
refactor, botreview: align result structs into one (#3120)
Browse files Browse the repository at this point in the history 
* refactor, botreview: align result structs into one

There was lots of duplication wrt storing review result data, i.e. there
was a struct specialized holding the result data per review strategy.
This was unnecessary since all these were doing the same thing, like
storing hunks that were special and storing whether the result would be
mergable right away.

These structs all have been replaced by the `BasicReviewResult` struct
that is used for every review strategy now.

Furthermore the review itself and the result interface and
implementation have been moved to their own file.

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

* refactor, botreview: remove CanMerge func and field

Since the sole presence of a `ShouldNotMergeReason` tells whether a pull
request can get merged, the `CanMerge` func and fields are redundant.

Therefore they have been completely removed.

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

* fix, botreview: append only non empty strings

ShouldNotMergeReason will be empty string by default, thus we need to
check before adding, or we will hold every PR.

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

* fix, bazel: update build file

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

---------

Signed-off-by: Daniel Hiller <dhiller@redhat.com>
  • Loading branch information
@dhiller
dhiller authored Dec 12, 2023
1 parent 6b46e52 commit 9bb2a11
Show file tree
Hide file tree
Showing 9 changed files with 155 additions and 198 deletions.
1 change: 1 addition & 0 deletions external-plugins/botreview/review/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ go_library(
"bump_kubevirtci.go",
"image_update.go",
"prow_autobump.go",
"result.go",
"review.go",
],
importpath = "kubevirt.io/project-infra/external-plugins/botreview/review",
Expand Down
53 changes: 1 addition & 52 deletions external-plugins/botreview/review/bump_kubevirtci.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,57 +44,6 @@ func init() {
bumpKubevirtCIClusterUpVersionMatcher = regexp.MustCompile(`(?m)^-[0-9]+-[a-z0-9]+$[\n]^\+[0-9]+-[a-z0-9]+$`)
}

type BumpKubevirtCIResult struct {
notMatchingHunks map[string][]*diff.Hunk
}

func (r BumpKubevirtCIResult) IsApproved() bool {
return len(r.notMatchingHunks) == 0
}

func (r BumpKubevirtCIResult) CanMerge() bool {
return true
}

func (r BumpKubevirtCIResult) String() string {
if r.IsApproved() {
return bumpKubevirtCIApproveComment
} else {
comment := bumpKubevirtCIDisapproveComment
for fileName, hunks := range r.notMatchingHunks {
comment += fmt.Sprintf("\nFile: `%s`", fileName)
for _, hunk := range hunks {
comment += fmt.Sprintf("\n```\n%s\n```", string(hunk.Body))
}
}
return comment
}
}

func (r *BumpKubevirtCIResult) AddReviewFailure(fileName string, hunks ...*diff.Hunk) {
if r.notMatchingHunks == nil {
r.notMatchingHunks = make(map[string][]*diff.Hunk)
}
if _, exists := r.notMatchingHunks[fileName]; !exists {
r.notMatchingHunks[fileName] = hunks
} else {
r.notMatchingHunks[fileName] = append(r.notMatchingHunks[fileName], hunks...)
}
}

func (r BumpKubevirtCIResult) ShortString() string {
if r.IsApproved() {
return bumpKubevirtCIApproveComment
} else {
comment := bumpKubevirtCIDisapproveComment
comment += fmt.Sprintf("\nFiles:")
for fileName := range r.notMatchingHunks {
comment += fmt.Sprintf("\n* `%s`", fileName)
}
return comment
}
}

type BumpKubevirtCI struct {
relevantFileDiffs []*diff.FileDiff
unwantedFiles map[string][]*diff.Hunk
Expand Down Expand Up @@ -129,7 +78,7 @@ func (t *BumpKubevirtCI) AddIfRelevant(fileDiff *diff.FileDiff) {
}

func (t *BumpKubevirtCI) Review() BotReviewResult {
result := &BumpKubevirtCIResult{}
result := NewCanMergeReviewResult(bumpKubevirtCIApproveComment, bumpKubevirtCIDisapproveComment)

for _, fileDiff := range t.relevantFileDiffs {
fileName := strings.TrimPrefix(fileDiff.NewName, "b/")
Expand Down
8 changes: 3 additions & 5 deletions external-plugins/botreview/review/bump_kubevirtci_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ func TestBumpKubevirtCI_Review(t1 *testing.T) {
tests := []struct {
name string
fields fields
want *BumpKubevirtCIResult
want BotReviewResult
}{
{
name: "simple prow autobump",
Expand All @@ -71,7 +71,7 @@ func TestBumpKubevirtCI_Review(t1 *testing.T) {
diffFilePathsToDiffs["testdata/kubevirtci-bump/hack_config-default.sh"],
},
},
want: &BumpKubevirtCIResult{},
want: newReviewResultWithData(bumpKubevirtCIApproveComment, bumpKubevirtCIDisapproveComment, nil, ""),
},
{
name: "mixed image bump",
Expand All @@ -82,9 +82,7 @@ func TestBumpKubevirtCI_Review(t1 *testing.T) {
diffFilePathsToDiffs["testdata/mixed_bump_prow_job.patch0"],
},
},
want: &BumpKubevirtCIResult{
notMatchingHunks: 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": diffFilePathsToDiffs["testdata/mixed_bump_prow_job.patch0"].Hunks}, ""),
},
}
for _, tt := range tests {
Expand Down
53 changes: 1 addition & 52 deletions external-plugins/botreview/review/image_update.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,57 +40,6 @@ var (
prowJobReleaseBranchFileNameMatcher = regexp.MustCompile(`.*/[\w-]+-[0-9-.]+\.yaml`)
)

type ProwJobImageUpdateResult struct {
notMatchingHunks map[string][]*diff.Hunk
}

func (r ProwJobImageUpdateResult) String() string {
if r.IsApproved() {
return prowJobImageUpdateApproveComment
} else {
comment := prowJobImageUpdateDisapproveComment
for fileName, hunks := range r.notMatchingHunks {
comment += fmt.Sprintf("\nFile: `%s`", fileName)
for _, hunk := range hunks {
comment += fmt.Sprintf("\n```\n%s\n```", string(hunk.Body))
}
}
return comment
}
}

func (r ProwJobImageUpdateResult) IsApproved() bool {
return len(r.notMatchingHunks) == 0
}

func (r ProwJobImageUpdateResult) CanMerge() bool {
return true
}

func (r *ProwJobImageUpdateResult) AddReviewFailure(fileName string, hunks ...*diff.Hunk) {
if r.notMatchingHunks == nil {
r.notMatchingHunks = make(map[string][]*diff.Hunk)
}
if _, exists := r.notMatchingHunks[fileName]; !exists {
r.notMatchingHunks[fileName] = hunks
} else {
r.notMatchingHunks[fileName] = append(r.notMatchingHunks[fileName], hunks...)
}
}

func (r ProwJobImageUpdateResult) ShortString() string {
if r.IsApproved() {
return prowJobImageUpdateApproveComment
} else {
comment := prowJobImageUpdateDisapproveComment
comment += fmt.Sprintf("\nFiles:")
for fileName := range r.notMatchingHunks {
comment += fmt.Sprintf("\n* `%s`", fileName)
}
return comment
}
}

type ProwJobImageUpdate struct {
relevantFileDiffs []*diff.FileDiff
notMatchingHunks []*diff.Hunk
Expand Down Expand Up @@ -132,7 +81,7 @@ func (t *ProwJobImageUpdate) AddIfRelevant(fileDiff *diff.FileDiff) {
}

func (t *ProwJobImageUpdate) Review() BotReviewResult {
result := &ProwJobImageUpdateResult{}
result := NewCanMergeReviewResult(prowJobImageUpdateApproveComment, prowJobImageUpdateDisapproveComment)

for _, fileDiff := range t.relevantFileDiffs {
fileName := strings.TrimPrefix(fileDiff.NewName, "b/")
Expand Down
9 changes: 4 additions & 5 deletions external-plugins/botreview/review/image_update_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func TestProwJobImageUpdate_Review(t1 *testing.T) {
tests := []struct {
name string
fields fields
want *ProwJobImageUpdateResult
want BotReviewResult
}{
{
name: "simple image bump",
Expand All @@ -60,7 +60,7 @@ func TestProwJobImageUpdate_Review(t1 *testing.T) {
diffFilePathsToDiffs["testdata/simple_bump-prow-job-images_sh.patch1"],
},
},
want: &ProwJobImageUpdateResult{},
want: NewCanMergeReviewResult(prowJobImageUpdateApproveComment, prowJobImageUpdateDisapproveComment),
},
{
name: "mixed image bump",
Expand All @@ -69,14 +69,13 @@ func TestProwJobImageUpdate_Review(t1 *testing.T) {
diffFilePathsToDiffs["testdata/mixed_bump_prow_job.patch0"],
},
},
want: &ProwJobImageUpdateResult{
notMatchingHunks: map[string][]*diff.Hunk{"github/ci/prow-deploy/files/jobs/kubevirt/kubevirt/kubevirt-presubmits.yaml": {diffFilePathsToDiffs["testdata/mixed_bump_prow_job.patch0"].Hunks[0]}},
},
want: newReviewResultWithData(prowJobImageUpdateApproveComment, prowJobImageUpdateDisapproveComment, map[string][]*diff.Hunk{"github/ci/prow-deploy/files/jobs/kubevirt/kubevirt/kubevirt-presubmits.yaml": {diffFilePathsToDiffs["testdata/mixed_bump_prow_job.patch0"].Hunks[0]}}, ""),
},
}
for _, tt := range tests {
t1.Run(tt.name, func(t1 *testing.T) {
t := &ProwJobImageUpdate{

relevantFileDiffs: tt.fields.relevantFileDiffs,
}
if got := t.Review(); !reflect.DeepEqual(got, tt.want) {
Expand Down
54 changes: 2 additions & 52 deletions external-plugins/botreview/review/prow_autobump.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ const (
I found suspicious hunks:
`
prowAutoBumpShouldNotMergeReason = "prow update should be merged at a point in time where it doesn't interfere with normal CI usage"
)

var prowAutobumpHunkBodyMatcher *regexp.Regexp
Expand All @@ -40,57 +41,6 @@ func init() {
prowAutobumpHunkBodyMatcher = regexp.MustCompile(`(?m)^(-[\s]+- image: [^\s]+$[\n]^\+[\s]+- image: [^\s]+|-[\s]+(image|clonerefs|initupload|entrypoint|sidecar): [^\s]+$[\n]^\+[\s]+(image|clonerefs|initupload|entrypoint|sidecar): [^\s]+)$`)
}

type ProwAutobumpResult struct {
notMatchingHunks map[string][]*diff.Hunk
}

func (r ProwAutobumpResult) String() string {
if len(r.notMatchingHunks) == 0 {
return prowAutobumpApproveComment
} else {
comment := prowAutobumpDisapproveComment
for fileName, hunks := range r.notMatchingHunks {
comment += fmt.Sprintf("\n%s", fileName)
for _, hunk := range hunks {
comment += fmt.Sprintf("\n```\n%s\n```", string(hunk.Body))
}
}
return comment
}
}

func (r ProwAutobumpResult) IsApproved() bool {
return len(r.notMatchingHunks) == 0
}

func (r ProwAutobumpResult) CanMerge() bool {
return false
}

func (r *ProwAutobumpResult) AddReviewFailure(fileName string, hunks ...*diff.Hunk) {
if r.notMatchingHunks == nil {
r.notMatchingHunks = make(map[string][]*diff.Hunk)
}
if _, exists := r.notMatchingHunks[fileName]; !exists {
r.notMatchingHunks[fileName] = hunks
} else {
r.notMatchingHunks[fileName] = append(r.notMatchingHunks[fileName], hunks...)
}
}

func (r ProwAutobumpResult) ShortString() string {
if r.IsApproved() {
return prowAutobumpApproveComment
} else {
comment := prowAutobumpDisapproveComment
comment += fmt.Sprintf("\nFiles:")
for fileName := range r.notMatchingHunks {
comment += fmt.Sprintf("\n* `%s`", fileName)
}
return comment
}
}

type ProwAutobump struct {
relevantFileDiffs []*diff.FileDiff
notMatchingHunks []*diff.Hunk
Expand All @@ -111,7 +61,7 @@ func (t *ProwAutobump) AddIfRelevant(fileDiff *diff.FileDiff) {
}

func (t *ProwAutobump) Review() BotReviewResult {
result := &ProwAutobumpResult{}
result := NewShouldNotMergeReviewResult(prowAutobumpApproveComment, prowAutobumpDisapproveComment, prowAutoBumpShouldNotMergeReason)

for _, fileDiff := range t.relevantFileDiffs {
fileName := strings.TrimPrefix(fileDiff.NewName, "b/")
Expand Down
12 changes: 5 additions & 7 deletions external-plugins/botreview/review/prow_autobump_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func TestProwAutobump_Review(t1 *testing.T) {
tests := []struct {
name string
fields fields
want *ProwAutobumpResult
want BotReviewResult
}{
{
name: "simple prow autobump",
Expand All @@ -78,7 +78,7 @@ func TestProwAutobump_Review(t1 *testing.T) {
diffFilePathsToDiffs["testdata/prow-autobump/github_ci_prow-deploy_kustom_overlays_ibmcloud-production_resources_prow-exporter-deployment.yaml"],
},
},
want: &ProwAutobumpResult{},
want: NewShouldNotMergeReviewResult(prowAutobumpApproveComment, prowAutobumpDisapproveComment, prowAutoBumpShouldNotMergeReason),
},
{
name: "prow autobump with crd update",
Expand All @@ -103,11 +103,9 @@ func TestProwAutobump_Review(t1 *testing.T) {
diffFilePathsToDiffs["testdata/prow-autobump/github_ci_prow-deploy_kustom_overlays_ibmcloud-production_resources_prow-exporter-deployment.yaml"],
},
},
want: &ProwAutobumpResult{
notMatchingHunks: map[string][]*diff.Hunk{
"github/ci/prow-deploy/kustom/base/manifests/test_infra/current/prowjob-crd/prowjob_customresourcedefinition.yaml": diffFilePathsToDiffs["testdata/prow-autobump/github_ci_prow-deploy_kustom_base_manifests_test_infra_current_prowjob-crd_prowjob_customresourcedefinition.yaml"].Hunks,
},
},
want: newReviewResultWithData(prowAutobumpApproveComment, prowAutobumpDisapproveComment, map[string][]*diff.Hunk{
"github/ci/prow-deploy/kustom/base/manifests/test_infra/current/prowjob-crd/prowjob_customresourcedefinition.yaml": diffFilePathsToDiffs["testdata/prow-autobump/github_ci_prow-deploy_kustom_base_manifests_test_infra_current_prowjob-crd_prowjob_customresourcedefinition.yaml"].Hunks,
}, prowAutoBumpShouldNotMergeReason),
},
}
for _, tt := range tests {
Expand Down
Loading

0 comments on commit 9bb2a11

Please sign in to comment.