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

Jump to bottom

Adding support for removing branch #382

Merged
merged 7 commits into from
Jan 2, 2024
Merged

Adding support for removing branch #382

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
05a400d
Adding support for removing branch
Sep 19, 2023
b85412e
resolving review comments
Oct 16, 2023
5a61b36
Fixing Readme doc
Nov 20, 2023
1b75206
Restructruing for optimizations
Nov 20, 2023
906c4b3
Updating the logging message for clear understanding
Nov 27, 2023
e6cc84a
Fixing test case
Dec 4, 2023
494bbe8
Changing Testing behaviour to test non existing branch
Dec 18, 2023
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
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
mohitsharma-in marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
_output
/coverage.out
.idea/*
mohitsharma-in marked this conversation as resolved.
Show resolved Hide resolved
10 changes: 8 additions & 2 deletions cmd/update-rules/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ Run the command line as:
```
update-rules -h

Usage: update-rules --branch BRANCH --rules PATHorURL [--go VERSION | -o PATH]
Usage: update-rules --branch BRANCH --rules PATHorURL [--go VERSION | -o PATH | -remove true/false]
mohitsharma-in marked this conversation as resolved.
Show resolved Hide resolved

Examples:
# Update rules for branch release-1.23 with go version 1.16.4
Expand All @@ -60,7 +60,10 @@ Run the command line as:
update-rules -branch release-1.23 -go 1.16.4 -rules https://raw.githubusercontent.com/kubernetes/kubernetes/master/staging/publishing/rules.yaml

# Update rules and export to /tmp/rules.yaml
update-rules -branch release-1.24 -go 1.17.1 -o /tmp/rules.yaml -rules /go/src/k8s.io/kubernetes/staging/publishing/rules.yaml
update-rules -branch release-1.24 -go 1.17.1 -o /tmp/rules.yaml -rules /go/src/k8s.io/kubernetes/staging/publishing/rules.yaml

# Remove rules and export to /tmp/rules.yaml
update-rules -branch release-1.24 -remove true -o /tmp/rules.yaml -rules /go/src/k8s.io/kubernetes/staging/publishing/rules.yaml
mohitsharma-in marked this conversation as resolved.
Show resolved Hide resolved

-alsologtostderr
log to standard error as well as files
Expand All @@ -76,6 +79,8 @@ Run the command line as:
log to standard error instead of files
-o string
Path to export the updated rules to, e.g. -o /tmp/rules.yaml
-remove
Remove old rules to deprecated branch
mohitsharma-in marked this conversation as resolved.
Show resolved Hide resolved
-rules string
[required] URL or Path of the rules file to update rules for, e.g. --rules path/or/url/to/rules/file.yaml
-stderrthreshold value
Expand All @@ -94,4 +99,5 @@ Run the command line as:
#### Optional flags:

- `-go` flag refers to golang version which should be pinned for given branch, if not given an empty string is set
- `-remove` flag refers to removing the branch from rules, defaults to false
mohitsharma-in marked this conversation as resolved.
Show resolved Hide resolved
- `-o` flag refers to output file where the processed rules should be exported, otherwise rules are printed on stdout
26 changes: 20 additions & 6 deletions cmd/update-rules/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ const GitDefaultBranch = "master"

type options struct {
branch string
remove bool
rulesFile string
goVersion string
out string
Expand All @@ -41,6 +42,7 @@ func parseOptions() options {
var o options
flag.StringVar(&o.branch, "branch", "", "[required] Branch to update rules for, e.g. --branch release-x.yy")
flag.StringVar(&o.rulesFile, "rules", "", "[required] URL or Path of the rules file to update rules for, e.g. --rules path/or/url/to/rules/file.yaml")
flag.BoolVar(&o.remove, "remove", false, "Remove old rules to deprecated branch")
flag.StringVar(&o.goVersion, "go", "", "Golang version to pin for this branch, e.g. --go 1.16.1")
flag.StringVar(&o.out, "o", "", "Path to export the updated rules to, e.g. -o /tmp/rules.yaml")

Expand All @@ -53,10 +55,13 @@ func parseOptions() options {
update-rules -branch release-1.21 -go 1.16.4 -rules https://raw.githubusercontent.com/kubernetes/kubernetes/master/staging/publishing/rules.yaml

# Update rules and export to /tmp/rules.yaml
update-rules -branch release-1.22 -go 1.17.1 -o /tmp/rules.yaml -rules /go/src/k8s.io/kubernetes/staging/publishing/rules.yaml`
update-rules -branch release-1.22 -go 1.17.1 -o /tmp/rules.yaml -rules /go/src/k8s.io/kubernetes/staging/publishing/rules.yaml

# Update rules to remove deprecated branch and export to /tmp/rules.yaml
update-rules -branch release-1.22 -remove true -o /tmp/rules.yaml -rules /go/src/k8s.io/kubernetes/staging/publishing/rules.yaml`

flag.Usage = func() {
fmt.Fprintf(os.Stdout, "\n Usage: update-rules --branch BRANCH --rules PATHorURL [--go VERSION | -o PATH]")
fmt.Fprintf(os.Stdout, "\n Usage: update-rules --branch BRANCH --rules PATHorURL [--go VERSION | -o PATH | -remove true/false]")
fmt.Fprintf(os.Stdout, "\n %s\n\n", examples)
flag.PrintDefaults()
}
Expand Down Expand Up @@ -86,8 +91,7 @@ func main() {
}

// update rules for all destination repos
UpdateRules(rules, o.branch, o.goVersion)

UpdateRules(rules, o.branch, o.goVersion, o.remove)
mohitsharma-in marked this conversation as resolved.
Show resolved Hide resolved
// validate rules after update
if err := config.Validate(rules); err != nil {
glog.Fatalf("update failed, found invalid rules after update: %v", err)
Expand Down Expand Up @@ -121,7 +125,7 @@ func load(rulesFile string) (*config.RepositoryRules, error) {
return rules, nil
}

func UpdateRules(rules *config.RepositoryRules, branch, goVer string) {
func UpdateRules(rules *config.RepositoryRules, branch, goVer string, removeRules bool) {
mohitsharma-in marked this conversation as resolved.
Show resolved Hide resolved
// run the update per destination repo in the rules
for j, r := range rules.Rules {
var mainBranchRuleFound bool
Expand All @@ -142,7 +146,9 @@ func UpdateRules(rules *config.RepositoryRules, branch, goVer string) {
}

// update the rules for branch and its dependencies
updateBranchRules(&newBranchRule, branch, goVer)
if !removeRules {
mohitsharma-in marked this conversation as resolved.
Show resolved Hide resolved
mohitsharma-in marked this conversation as resolved.
Show resolved Hide resolved
updateBranchRules(&newBranchRule, branch, goVer)
}

var branchRuleExists bool
// if the target branch rules already exists, update it
mohitsharma-in marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -154,6 +160,14 @@ func UpdateRules(rules *config.RepositoryRules, branch, goVer string) {
break
}
}
for i, br := range r.Branches {
if removeRules && br.Name == branch {
glog.Infof("Will remove rule for %s %s", branch, r.DestinationRepository)
mohitsharma-in marked this conversation as resolved.
Show resolved Hide resolved
r.Branches = append(r.Branches[:i], r.Branches[i+1:]...)
branchRuleExists = true
break
}
}
// new rules, append to destination's branches
if !branchRuleExists {
r.Branches = append(r.Branches, newBranchRule)
Expand Down
39 changes: 38 additions & 1 deletion cmd/update-rules/main_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ func TestUpdateRules(t *testing.T) {
if err != nil {
t.Errorf("error loading test rules file %v", err)
}
UpdateRules(rules, tt.branch, tt.goVersion)
UpdateRules(rules, tt.branch, tt.goVersion, false)

for _, repoRule := range rules.Rules {
var masterRulePresent, branchRulePresent bool
Expand Down Expand Up @@ -163,3 +163,40 @@ func TestUpdateRules(t *testing.T) {
})
}
}

func TestRemoveRules(t *testing.T) {
tests := []struct {
name string
branch string
goVersion string
mohitsharma-in marked this conversation as resolved.
Show resolved Hide resolved
}{
{
"release-1.20",
mohitsharma-in marked this conversation as resolved.
Show resolved Hide resolved
"release-1.XY",
"1.17.1",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few changes are needed on the tests.

  1. Can you give a proper name to the test?
  2. 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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

},
{
"release-1.19",
"release-1.XY",
"1.17.1",
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
rules, err := load(testdataRules)
if err != nil {
t.Errorf("error loading test rules file %v", err)
}
UpdateRules(rules, tt.branch, tt.goVersion, true)

for _, repoRule := range rules.Rules {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have and recommendations around this

for _, branchRule := range repoRule.Branches {
if branchRule.Name == tt.branch {
t.Errorf("Failed to delete %s branch rule from for repo %s", tt.name, repoRule.DestinationRepository)
}
}
}
})
}
}
Loading