Skip to content

Commit

Permalink
Only check require status checks if list is set
Browse files Browse the repository at this point in the history
  • Loading branch information
@yorinasub17 @jeffmendoza
yorinasub17 authored and jeffmendoza committed Mar 24, 2022
1 parent 1765e90 commit ddf64e4
Show file tree
Hide file tree
Showing 2 changed files with 110 additions and 60 deletions.
104 changes: 50 additions & 54 deletions pkg/policies/branch/branch.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ type OrgConfig struct {
BlockForce bool `yaml:"blockForce"`

// RequireUpToDateBranch : set to true to require that branches must be up
// to date before merging. Default false.
// to date before merging. Only used if RequireStatusChecks is set. Default true.
RequireUpToDateBranch bool `yaml:"requireUpToDateBranch"`

// RequireStatusChecks is a list of status checks (by name) that are required in
Expand Down Expand Up @@ -98,8 +98,7 @@ type RepoConfig struct {
// BlockForce overrides the same setting in org-level, only if present.
BlockForce *bool `yaml:"blockForce"`

// RequireUpToDateBranch : set to true to require that branches must be up
// to date before merging. Default false.
// RequireUpToDateBranch overrides the same setting in org-level, only if present.
RequireUpToDateBranch *bool `yaml:"requireUpToDateBranch"`

// RequireStatusChecks is a list of status checks (by name) that are required in
Expand Down Expand Up @@ -288,39 +287,32 @@ func check(ctx context.Context, rep repositories, c *github.Client, owner,
d.BlockForce = false
}
}
rsc := p.GetRequiredStatusChecks()
if rsc != nil {
d.RequireUpToDateBranch = rsc.Strict
if mc.RequireUpToDateBranch && !rsc.Strict {
text = text +
fmt.Sprintf("Require up to date branch not configured for branch %v\n",
b)
pass = false
}
d.RequireStatusChecks = rsc.Contexts
c := make(map[string]struct{}, len(rsc.Contexts))
for _, ctx := range rsc.Contexts {
c[ctx] = struct{}{}
}
for _, check := range mc.RequireStatusChecks {
if _, ok := c[check]; !ok {
if len(mc.RequireStatusChecks) > 0 {
rsc := p.GetRequiredStatusChecks()
if rsc != nil {
d.RequireUpToDateBranch = rsc.Strict
if mc.RequireUpToDateBranch && !rsc.Strict {
text = text +
fmt.Sprintf("Status check %v not required for branch %v\n",
check, b)
fmt.Sprintf("Require up to date branch not configured for branch %v\n",
b)
pass = false
}
}
} else {
d.RequireUpToDateBranch = false
if mc.RequireUpToDateBranch {
text = text +
fmt.Sprintf("Require up to date branch not configured for branch %v\n",
b)
pass = false
}
if len(mc.RequireStatusChecks) > 0 {
d.RequireStatusChecks = rsc.Contexts
c := make(map[string]struct{}, len(rsc.Contexts))
for _, ctx := range rsc.Contexts {
c[ctx] = struct{}{}
}
for _, check := range mc.RequireStatusChecks {
if _, ok := c[check]; !ok {
text = text +
fmt.Sprintf("Status check %v not found for branch %v\n",
check, b)
pass = false
}
}
} else {
text = text +
fmt.Sprintf("No status checks required for branch %v\n", b)
fmt.Sprintf("Status checks required by policy, but none found for branch %v\n", b)
pass = false
}
}
Expand Down Expand Up @@ -376,7 +368,7 @@ func fix(ctx context.Context, rep repositories, c *github.Client,
}
pr.RequiredPullRequestReviews = rq
}
if mc.RequireUpToDateBranch || len(mc.RequireStatusChecks) > 0 {
if len(mc.RequireStatusChecks) > 0 {
rsc := &github.RequiredStatusChecks{
Strict: mc.RequireUpToDateBranch,
// Can't be set to nil, so force non-nil value.
Expand Down Expand Up @@ -462,33 +454,36 @@ func fix(ctx context.Context, rep repositories, c *github.Client,
update = true
}
}
if pr.RequiredStatusChecks == nil && (mc.RequireUpToDateBranch || len(mc.RequireStatusChecks) > 0) {
rsc := &github.RequiredStatusChecks{
Strict: mc.RequireUpToDateBranch,
// Can't be set to nil, so force non-nil value.
Contexts: append([]string{}, mc.RequireStatusChecks...),
}
pr.RequiredStatusChecks = rsc
update = true
} else if mc.RequireUpToDateBranch || len(mc.RequireStatusChecks) > 0 {
if mc.RequireUpToDateBranch && !pr.RequiredStatusChecks.Strict {
pr.RequiredStatusChecks.Strict = true
if len(mc.RequireStatusChecks) > 0 {
if pr.RequiredStatusChecks == nil {
rsc := &github.RequiredStatusChecks{
Strict: mc.RequireUpToDateBranch,
// Can't be set to nil, so force non-nil value.
Contexts: append([]string{}, mc.RequireStatusChecks...),
}
pr.RequiredStatusChecks = rsc
update = true
}
if len(mc.RequireStatusChecks) > 0 {
} else {
if mc.RequireUpToDateBranch && !pr.RequiredStatusChecks.Strict {
pr.RequiredStatusChecks.Strict = true
update = true
}
allContexts := make(map[string]struct{}, len(pr.RequiredStatusChecks.Contexts))
for _, ctx := range pr.RequiredStatusChecks.Contexts {
allContexts[ctx] = struct{}{}
}
for _, ctx := range mc.RequireStatusChecks {
allContexts[ctx] = struct{}{}
// Only mark for update if there are status checks required, but not already set.
if _, ok := allContexts[ctx]; !ok {
allContexts[ctx] = struct{}{}
update = true
}
}
pr.RequiredStatusChecks.Contexts = make([]string, 0)
for ctx := range allContexts {
pr.RequiredStatusChecks.Contexts = append(pr.RequiredStatusChecks.Contexts, ctx)
}
sort.Strings(pr.RequiredStatusChecks.Contexts)
update = true
}
}
if update {
Expand Down Expand Up @@ -517,12 +512,13 @@ func (b Branch) GetAction(ctx context.Context, c *github.Client, owner, repo str

func getConfig(ctx context.Context, c *github.Client, owner, repo string) (*OrgConfig, *RepoConfig) {
oc := &OrgConfig{ // Fill out non-zero defaults
Action: "log",
EnforceDefault: true,
RequireApproval: true,
ApprovalCount: 1,
DismissStale: true,
BlockForce: true,
Action: "log",
EnforceDefault: true,
RequireApproval: true,
ApprovalCount: 1,
DismissStale: true,
BlockForce: true,
RequireUpToDateBranch: true,
}
if err := configFetchConfig(ctx, c, owner, "", configFile, true, oc); err != nil {
log.Error().
Expand Down
66 changes: 60 additions & 6 deletions pkg/policies/branch/branch_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -214,8 +214,8 @@ func TestCheck(t *testing.T) {
cofigEnabled: true,
Exp: policydef.Result{
Enabled: true,
Pass: false,
NotifyText: "Require up to date branch not configured for branch main\n",
Pass: true,
NotifyText: "",
Details: map[string]details{
"main": details{
PRReviews: true,
Expand All @@ -239,6 +239,7 @@ func TestCheck(t *testing.T) {
DismissStale: true,
BlockForce: true,
RequireUpToDateBranch: true,
RequireStatusChecks: []string{"mycheck", "theothercheck"},
},
Repo: RepoConfig{},
Prot: map[string]github.Protection{
Expand All @@ -251,7 +252,8 @@ func TestCheck(t *testing.T) {
Enabled: false,
},
RequiredStatusChecks: &github.RequiredStatusChecks{
Strict: false,
Strict: false,
Contexts: []string{"mycheck", "theothercheck"},
},
},
},
Expand All @@ -267,6 +269,7 @@ func TestCheck(t *testing.T) {
DismissStale: true,
BlockForce: true,
RequireUpToDateBranch: false,
RequireStatusChecks: []string{"mycheck", "theothercheck"},
},
},
},
Expand Down Expand Up @@ -300,7 +303,7 @@ func TestCheck(t *testing.T) {
Exp: policydef.Result{
Enabled: true,
Pass: false,
NotifyText: "No status checks required for branch main\n",
NotifyText: "Status checks required by policy, but none found for branch main\n",
Details: map[string]details{
"main": details{
PRReviews: true,
Expand Down Expand Up @@ -344,7 +347,7 @@ func TestCheck(t *testing.T) {
Exp: policydef.Result{
Enabled: true,
Pass: false,
NotifyText: "Status check theothercheck not required for branch main\n",
NotifyText: "Status check theothercheck not found for branch main\n",
Details: map[string]details{
"main": details{
PRReviews: true,
Expand Down Expand Up @@ -718,6 +721,30 @@ func TestFix(t *testing.T) {
},
},
cofigEnabled: true,
Exp: map[string]github.ProtectionRequest{},
},
{
Name: "RequireUpToDateBranch",
Org: OrgConfig{
EnforceDefault: true,
RequireUpToDateBranch: true,
RequireStatusChecks: []string{"mycheck", "theothercheck"},
},
Repo: RepoConfig{},
Prot: map[string]github.Protection{
"main": github.Protection{
AllowForcePushes: &github.AllowForcePushes{
Enabled: false,
},
EnforceAdmins: &github.AdminEnforcement{
Enabled: false,
},
RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{
RequiredApprovingReviewCount: 0,
},
},
},
cofigEnabled: true,
Exp: map[string]github.ProtectionRequest{
"main": github.ProtectionRequest{
AllowForcePushes: &flse,
Expand All @@ -726,7 +753,7 @@ func TestFix(t *testing.T) {
},
RequiredStatusChecks: &github.RequiredStatusChecks{
Strict: true,
Contexts: []string{},
Contexts: []string{"mycheck", "theothercheck"},
},
},
},
Expand Down Expand Up @@ -803,6 +830,33 @@ func TestFix(t *testing.T) {
},
},
},
{
Name: "NoChangeToRequireStatusChecks",
Org: OrgConfig{
EnforceDefault: true,
RequireStatusChecks: []string{"mycheck", "theothercheck"},
},
Repo: RepoConfig{},
Prot: map[string]github.Protection{
"main": github.Protection{
AllowForcePushes: &github.AllowForcePushes{
Enabled: false,
},
EnforceAdmins: &github.AdminEnforcement{
Enabled: false,
},
RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{
RequiredApprovingReviewCount: 0,
},
RequiredStatusChecks: &github.RequiredStatusChecks{
Strict: false,
Contexts: []string{"mycheck", "theothercheck"},
},
},
},
cofigEnabled: true,
Exp: map[string]github.ProtectionRequest{},
},
}
get = func(context.Context, string, string) (*github.Repository,
*github.Response, error) {
Expand Down

0 comments on commit ddf64e4

Please sign in to comment.