Adding support for removing branch #382

mohitsharma-in · 2023-09-19T11:46:31Z

Adding support for removing branch
Fixes #377

Cc: @akhilerm

akhilerm · 2023-09-19T11:51:07Z

/cc @akhilerm

Adding support for removing branch

mohitsharma-in · 2023-09-25T13:03:16Z

@akhilerm Please Review

akhilerm

Provided a few comments.

.gitignore

cmd/update-rules/README.md

cmd/update-rules/main.go

akhilerm · 2023-10-05T05:14:47Z

@mohitsharma-in kubernetes/kubernetes#120822 has been merged. So now you wont see the diff in the rules related to the new field.

akhilerm · 2023-10-23T10:10:32Z

@mohitsharma-in Is this ready for review ?

mohitsharma-in · 2023-10-26T11:21:30Z

@mohitsharma-in Is this ready for review ?

@akhilerm
Please review the code i will update readme once the code is good

akhilerm

Given a few comments.

.gitignore

cmd/update-rules/main.go

.idea/workspace.xml

bin/golangci-lint

test-update-rule/new-rules.yaml

akhilerm

A few changes are needed on the test cases.

Sorry @mohitsharma-in I missed to review the test file during the last round of review.

akhilerm · 2023-12-04T06:22:11Z

cmd/update-rules/main_test.go

+			"release-1.20",
+			"release-1.XY",
+			"1.17.1",


A few changes are needed on the tests.

Can you give a proper name to the test?

Since release-1.XY does not exist in the testdata/rules.yaml, if we try to delete that branch, the test will always pass. We need to add another case in which we try to delete a branch that already exists in the rules.yaml file and make sure that after the UpdateRules() func, the branch is properly deleted.

fixed 1 and 2
For 2 point we already have a loop which checks and returns error if branch is not deleted

We need the inverse of that also, Like to make sure that there are no changes to any of the rules. see the below comment.

cmd/update-rules/main_test.go

akhilerm · 2023-12-04T08:29:04Z

cmd/update-rules/main_test.go

+			}
+			UpdateRules(rules, tt.branch, tt.goVersion, true)
+
+			for _, repoRule := range rules.Rules {


One more check needs to be done to make sure that, if the branch to be deleted was a non-existent branch, there are no changes to the rules that has been loaded from testdataRules

we have the error logged in the below loop for the same if it failed to delete the branch do you mean should we check if branch exist or not instead before deleting?

we have the error logged in the below loop for the same if it failed to delete the branch

this is correct.

do you mean should we check if branch exist or not instead before deleting?

no. The test should check that, if a non existent branch is tried for deletion, it doesnt affect rest of the rules.

Do you have and recommendations around this

mohitsharma-in · 2023-12-11T04:57:42Z

/retest

cmd/update-rules/main.go

akhilerm

/lgtm
/assign @dims @nikhita for approval

k8s-ci-robot · 2023-12-18T07:29:31Z

@akhilerm: GitHub didn't allow me to assign the following users: for, approval.

Note that only kubernetes members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/lgtm
/assign @dims @nikhita for approval

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

dims · 2024-01-02T15:53:11Z

/approve
/lgtm

k8s-ci-robot · 2024-01-02T15:53:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akhilerm, dims, mohitsharma-in

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [dims]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 19, 2023

k8s-ci-robot requested review from jimangel and jrsapi September 19, 2023 11:46

k8s-ci-robot requested a review from akhilerm September 19, 2023 11:51

mohitsharma-in force-pushed the adding-branch-removal-support branch from b5fddcb to f3a5dae Compare September 19, 2023 12:02

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 19, 2023

mohitsharma-in force-pushed the adding-branch-removal-support branch 2 times, most recently from 7d2353d to 5239145 Compare September 21, 2023 06:54

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 21, 2023

Adding support for removing branch

05a400d

Adding support for removing branch

mohitsharma-in force-pushed the adding-branch-removal-support branch from 5239145 to 05a400d Compare September 21, 2023 07:09

mohitsharma-in changed the title ~~[WIP] Adding support for removing branch~~ Adding support for removing branch Sep 22, 2023

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 22, 2023

akhilerm reviewed Sep 25, 2023

View reviewed changes

mohitsharma-in force-pushed the adding-branch-removal-support branch from 936cee1 to 47b8753 Compare October 16, 2023 07:32

akhilerm suggested changes Oct 26, 2023

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

cmd/update-rules/main.go Outdated Show resolved Hide resolved

cmd/update-rules/main.go Outdated Show resolved Hide resolved

cmd/update-rules/main.go Outdated Show resolved Hide resolved

mohitsharma-in force-pushed the adding-branch-removal-support branch 3 times, most recently from 371aea9 to 69f351f Compare October 31, 2023 11:28

resolving review comments

b85412e

mohitsharma-in force-pushed the adding-branch-removal-support branch from 69f351f to b85412e Compare November 20, 2023 06:26

akhilerm suggested changes Nov 27, 2023

View reviewed changes

.idea/workspace.xml Outdated Show resolved Hide resolved

bin/golangci-lint Outdated Show resolved Hide resolved

test-update-rule/new-rules.yaml Outdated Show resolved Hide resolved

mohitsharma-in force-pushed the adding-branch-removal-support branch from df5190e to 02a4bf5 Compare November 27, 2023 10:24

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 27, 2023

mohitsharma-in force-pushed the adding-branch-removal-support branch 2 times, most recently from 24490f5 to aa70fd9 Compare November 27, 2023 13:19

Updating the logging message for clear understanding

906c4b3

mohitsharma-in force-pushed the adding-branch-removal-support branch from aa70fd9 to 906c4b3 Compare December 4, 2023 05:09

akhilerm suggested changes Dec 4, 2023

View reviewed changes

Fixing test case

e6cc84a

mohitsharma-in force-pushed the adding-branch-removal-support branch from 0f9b654 to e6cc84a Compare December 11, 2023 04:52

akhilerm mentioned this pull request Dec 14, 2023

Drop publishing-bot rules for the release-1.25 branch kubernetes/kubernetes#122321

Merged

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 18, 2023

akhilerm reviewed Dec 18, 2023

View reviewed changes

cmd/update-rules/main.go Outdated Show resolved Hide resolved

Changing Testing behaviour to test non existing branch

494bbe8

mohitsharma-in force-pushed the adding-branch-removal-support branch from 5bb3ffa to 494bbe8 Compare December 18, 2023 06:50

akhilerm reviewed Dec 18, 2023

View reviewed changes

k8s-ci-robot assigned dims and nikhita Dec 18, 2023

k8s-ci-robot assigned akhilerm Dec 18, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 18, 2023

akhilerm approved these changes Dec 19, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 2, 2024

k8s-ci-robot merged commit 3cb4242 into kubernetes:master Jan 2, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for removing branch #382

Adding support for removing branch #382

mohitsharma-in commented Sep 19, 2023

akhilerm commented Sep 19, 2023

mohitsharma-in commented Sep 25, 2023

akhilerm left a comment

akhilerm commented Oct 5, 2023

akhilerm commented Oct 23, 2023

mohitsharma-in commented Oct 26, 2023 •

edited

Loading

akhilerm left a comment

akhilerm left a comment

akhilerm Dec 4, 2023

mohitsharma-in Dec 4, 2023

akhilerm Dec 4, 2023

akhilerm Dec 4, 2023

mohitsharma-in Dec 11, 2023

akhilerm Dec 11, 2023

mohitsharma-in Dec 15, 2023

mohitsharma-in commented Dec 11, 2023

akhilerm left a comment

k8s-ci-robot commented Dec 18, 2023

dims commented Jan 2, 2024

k8s-ci-robot commented Jan 2, 2024

Adding support for removing branch #382

Adding support for removing branch #382

Conversation

mohitsharma-in commented Sep 19, 2023

akhilerm commented Sep 19, 2023

mohitsharma-in commented Sep 25, 2023

akhilerm left a comment

Choose a reason for hiding this comment

akhilerm commented Oct 5, 2023

akhilerm commented Oct 23, 2023

mohitsharma-in commented Oct 26, 2023 • edited Loading

akhilerm left a comment

Choose a reason for hiding this comment

akhilerm left a comment

Choose a reason for hiding this comment

akhilerm Dec 4, 2023

Choose a reason for hiding this comment

mohitsharma-in Dec 4, 2023

Choose a reason for hiding this comment

akhilerm Dec 4, 2023

Choose a reason for hiding this comment

akhilerm Dec 4, 2023

Choose a reason for hiding this comment

mohitsharma-in Dec 11, 2023

Choose a reason for hiding this comment

akhilerm Dec 11, 2023

Choose a reason for hiding this comment

mohitsharma-in Dec 15, 2023

Choose a reason for hiding this comment

mohitsharma-in commented Dec 11, 2023

akhilerm left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 18, 2023

dims commented Jan 2, 2024

k8s-ci-robot commented Jan 2, 2024

mohitsharma-in commented Oct 26, 2023 •

edited

Loading