Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix, botreview, strategy: ignore unrelated files in kubevirtci-bump #3123

Closed
wants to merge 1 commit into from
Closed

fix, botreview, strategy: ignore unrelated files in kubevirtci-bump #3123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 11 additions & 19 deletions external-plugins/botreview/review/bump_kubevirtci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func init() {

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

func (t *BumpKubevirtCI) IsRelevant() bool {
Expand All @@ -56,25 +56,17 @@ 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)
}
}
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 {
Expand Down Expand Up @@ -103,8 +95,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
Expand Down
69 changes: 65 additions & 4 deletions external-plugins/botreview/review/bump_kubevirtci_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,25 @@ 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",
}
entries, err := os.ReadDir("testdata/kubevirtci-bump")
if err != nil {
t1.Errorf("failed to read files: %v", err)
Expand Down Expand Up @@ -58,7 +76,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"],
Expand All @@ -74,15 +92,58 @@ 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,
}, ""),
},
}
for _, tt := range tests {
Expand Down
2 changes: 1 addition & 1 deletion external-plugins/botreview/review/review.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func GuessReviewTypes(fileDiffs []*diff.FileDiff) []KindOfChange {
kindOfChange.AddIfRelevant(fileDiff)
}
}
result := []KindOfChange{}
var result []KindOfChange
for _, t := range possibleReviewTypes {
if t.IsRelevant() {
result = append(result, t)
Expand Down
68 changes: 64 additions & 4 deletions external-plugins/botreview/review/review_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,28 @@ func TestGuessReviewTypes(t *testing.T) {
"testdata/move_prometheus_stack.patch0",
"testdata/move_prometheus_stack.patch1",
"testdata/cdi_arm_release.patch0",
"testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch0",
"testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch1",
"testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch2",
"testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch3",
"testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch4",
"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",
}
diffFilePathsToDiffs := map[string]*diff.FileDiff{}
for _, diffFile := range diffFilePaths {
Expand Down Expand Up @@ -87,23 +109,61 @@ func TestGuessReviewTypes(t *testing.T) {
},
},
{
name: "non image bump should not yield a change",
name: "non image bump (move_prometheus_stack) should not yield a change",
args: args{
fileDiffs: []*diff.FileDiff{
diffFilePathsToDiffs["testdata/move_prometheus_stack.patch0"],
diffFilePathsToDiffs["testdata/move_prometheus_stack.patch1"],
},
},
want: []KindOfChange{},
want: nil,
},
{
name: "non image bump should not yield a change 2",
name: "non image bump (cdi_arm_release) should not yield a change 2",
args: args{
fileDiffs: []*diff.FileDiff{
diffFilePathsToDiffs["testdata/cdi_arm_release.patch0"],
},
},
want: []KindOfChange{},
want: nil,
},
{
name: "non kubevirtci bump (eviction-strategy-doc) should not yield a change",
args: args{
fileDiffs: []*diff.FileDiff{
diffFilePathsToDiffs["testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch0"],
diffFilePathsToDiffs["testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch1"],
diffFilePathsToDiffs["testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch2"],
diffFilePathsToDiffs["testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch3"],
diffFilePathsToDiffs["testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch4"],
},
},
want: nil,
},
{
name: "non kubevirtci bump (fix-containerdisks-migration) should not yield a change",
args: args{
fileDiffs: []*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: nil,
},
}
for _, tt := range tests {
Expand Down
13 changes: 13 additions & 0 deletions ...ins/botreview/review/testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch0
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
a/api/openapi-spec/swagger.json b/api/openapi-spec/swagger.json
index 44873ea7f..f69e61964 100644
--- a/api/openapi-spec/swagger.json
+++ b/api/openapi-spec/swagger.json
@@ -20275,7 +20275,7 @@
"$ref": "#/definitions/v1.DomainSpec"
},
"evictionStrategy": {
- "description": "EvictionStrategy can be set to \"LiveMigrate\" if the VirtualMachineInstance should be migrated instead of shut-off in case of a node drain.",
+ "description": "EvictionStrategy describes the strategy to follow when a node drain occurs. The possible options are: - \"None\": No action will be taken, according to the specified 'RunStrategy' the VirtualMachine will be restarted or shutdown. - \"LiveMigrate\": the VirtualMachineInstance will be migrated instead of being shutdown. - \"LiveMigrateIfPossible\": the same as \"LiveMigrate\" but only if the VirtualMachine is Live-Migratable, otherwise it will behave as \"None\". - \"External\": the VirtualMachineInstance will be protected by a PDB and `vmi.Status.EvacuationNodeName` will be set on eviction. This is mainly useful for cluster-api-provider-kubevirt (capk) which needs a way for VMI's to be blocked from eviction, yet signal capk that eviction has been called on the VMI so the capk controller can handle tearing the VMI down. Details can be found in the commit description https://github.com/kubevirt/kubevirt/commit/c1d77face705c8b126696bac9a3ee3825f27f1fa.",
"type": "string"
},
"hostname": {
Loading