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.

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

Adding a Tide Configuration for checking only github required checks. #7457

Closed
wants to merge 9 commits into from
Closed
4 changes: 4 additions & 0 deletions prow/cmd/branchprotector/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ branch-protection:
# Ensure that the extra-process-followed github status context passes.
# In addition, adds any required prow jobs (aka always_run: true)
require-contexts: ["extra-process-followed"]
# Tell Tide to merge when unspecified contexts are pending or failing
skip-unknown-contexts: true

presubmits:
kubernetes/test-infra:
Expand Down Expand Up @@ -79,6 +81,8 @@ branch-protection:
protect-by-default: true
# Require the foo status context
require-contexts: ["foo"]
# Tell Tide to merge when unspecified contexts are pending or failing
skip-unknown-contexts: true
different-org:
# Inherits protect-by-default: true setting from above
```
Expand Down
81 changes: 81 additions & 0 deletions prow/cmd/tide/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
# Tide Documentation
Copy link
Contributor

Choose a reason for hiding this comment

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

Great documentation! Consider putting the README in its own PR and just adding the skip optional tag in this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that will be your insentive to get this PR in.


Tide merges PR that match a given sets of criteria

## Tide configuration

Extend the primary prow [`config.yaml`] document to include a top-level
`tide` key that looks like the following:

```yaml

tide:
queries:
...
branchProtection:
...
merge_method:
...


presubmits:
kubernetes/test-infra:
- name: fancy-job-name
context: fancy-job-name
always_run: true
spec: # podspec that runs job
```


### Merging Options

Tide supports all 3 github merging options:

* squash
* merge
* rebase

A merge method can be set for repo or per org.

Example:

```yaml
tide:
...
merge_method:
org1: squash
org2/repo1: rebase
org2/repo2: merge
```

### Queries Configuration

Queries are using github queries to find PRs to be merged. Multiple queries can be defined for a single repo. Queries support filtering per existing and missing labels. In order to filter PRs that have been approved, use the reviewApprovedRequired.

```yaml
tide:
queries:
...
- repos:
- org1/repo1
- org2/repo2
labels:
- labelThatMustsExists
- OtherLabelThatMustsExist
missingLabels:
- labelThatShouldNotBePresent
# If you want github approval
reviewApprovedRequired: true
```

### Branch Protection Options
Branch Protection options are use to enforce github branch protection.
A PR will be merged when all checks are passing. If we want to skip unknown checks (ie checks that are not in Prow Config), we can set skip-unknown-contexts to true. This option can be set globally or per org, repo and branch.

Example:

```yaml
branch-protection:
# Tell Tide to merge when unspecified contexts are pending or failing
skip-unknown-contexts: true
```
31 changes: 20 additions & 11 deletions prow/config/branch_protection.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ type Policy struct {
// TODO(fejta): add all protection options
Contexts []string `json:"require-contexts,omitempty"`
Pushers []string `json:"allow-push,omitempty"`
// SkipUnknownContexts will consider undefined contexts in Prow Config
// (Presubmits and branch protection) to be optional for a merge by Tide.
SkipUnknownContexts *bool `json:"skip-unknown-contexts,omitempty"`
}

// selectBool returns the child argument if set, otherwise the parent
Expand All @@ -53,9 +56,10 @@ func unionStrings(parent, child []string) []string {
// apply returns a policy that merges the child into the parent
func (parent Policy) Apply(child Policy) Policy {
return Policy{
Protect: selectBool(parent.Protect, child.Protect),
Contexts: unionStrings(parent.Contexts, child.Contexts),
Pushers: unionStrings(parent.Pushers, child.Pushers),
Protect: selectBool(parent.Protect, child.Protect),
Contexts: unionStrings(parent.Contexts, child.Contexts),
Pushers: unionStrings(parent.Pushers, child.Pushers),
SkipUnknownContexts: selectBool(parent.SkipUnknownContexts, child.SkipUnknownContexts),
}
}

Expand All @@ -76,6 +80,7 @@ type Repo struct {
Branches map[string]Branch `json:"branches,omitempty"`
}

// TODO(fejta): Remove Branch and only use Policy ?
type Branch struct {
Policy
}
Expand All @@ -99,7 +104,7 @@ func (c *Config) GetBranchProtection(org, repo, branch string) (*Policy, error)
}

// Automatically require any required prow jobs
if prowContexts := branchRequirements(org, repo, branch, c.Presubmits); len(prowContexts) > 0 {
if prowContexts, _ := BranchRequirements(org, repo, branch, c.Presubmits); len(prowContexts) > 0 {
// Error if protection is disabled
if policy.Protect != nil && !*policy.Protect {
return nil, fmt.Errorf("required prow jobs require branch protection")
Expand Down Expand Up @@ -132,8 +137,7 @@ func (c *Config) GetBranchProtection(org, repo, branch string) (*Policy, error)
return &policy, nil
}

func jobRequirements(jobs []Presubmit, branch string, after bool) []string {
var required []string
func jobRequirements(jobs []Presubmit, branch string, after bool) (required, optional []string) {
for _, j := range jobs {
if !j.Brancher.RunsAgainstBranch(branch) {
continue
Expand All @@ -142,19 +146,24 @@ func jobRequirements(jobs []Presubmit, branch string, after bool) []string {
if !after && !j.AlwaysRun && j.RunIfChanged == "" {
continue // No
}
if !j.SkipReport && !j.Optional { // This job needs a context
if j.ContextRequired() { // This job needs a context
required = append(required, j.Context)
} else {
optional = append(optional, j.Context)
}
// Check which children require contexts
required = append(required, jobRequirements(j.RunAfterSuccess, branch, true)...)
r, o := jobRequirements(j.RunAfterSuccess, branch, true)
required = append(required, r...)
optional = append(optional, o...)
}
return required
return
}

func branchRequirements(org, repo, branch string, presubmits map[string][]Presubmit) []string {
// BranchRequirements returns required and optional presubmits prow jobs for a given org, repo branch.
func BranchRequirements(org, repo, branch string, presubmits map[string][]Presubmit) (required, optional []string) {
p, ok := presubmits[org+"/"+repo]
if !ok {
return nil
return
}
return jobRequirements(p, branch, false)
}
4 changes: 2 additions & 2 deletions prow/config/branch_protection_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -298,11 +298,11 @@ func TestJobRequirements(t *testing.T) {
}

for _, tc := range cases {
masterActual := jobRequirements(tc.config, "master", false)
masterActual, _ := jobRequirements(tc.config, "master", false)
if !reflect.DeepEqual(masterActual, tc.masterExpected) {
t.Errorf("branch: master - %s: actual %v != expected %v", tc.name, masterActual, tc.masterExpected)
}
otherActual := jobRequirements(tc.config, "other", false)
otherActual, _ := jobRequirements(tc.config, "other", false)
if !reflect.DeepEqual(masterActual, tc.masterExpected) {
t.Errorf("branch: other - %s: actual %v != expected %v", tc.name, otherActual, tc.otherExpected)
}
Expand Down
8 changes: 8 additions & 0 deletions prow/config/jobs.go
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,14 @@ func (ps Presubmit) TriggerMatches(body string) bool {
return ps.re.MatchString(body)
}

// ContextRequired checks whether a context is required from github points of view (required check).
func (ps Presubmit) ContextRequired() bool {
if ps.Optional || ps.SkipReport {
return false
}
return true
}

type ChangedFilesProvider func() ([]string, error)

func matching(j Presubmit, body string, testAll bool) []Presubmit {
Expand Down
10 changes: 8 additions & 2 deletions prow/tide/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")

go_library(
name = "go_default_library",
srcs = ["tide.go"],
srcs = [
"contextregister.go",
"tide.go",
],
importpath = "k8s.io/test-infra/prow/tide",
visibility = ["//visibility:public"],
deps = [
Expand Down Expand Up @@ -33,7 +36,10 @@ filegroup(

go_test(
name = "go_default_test",
srcs = ["tide_test.go"],
srcs = [
"contextregister_test.go",
"tide_test.go",
],
embed = [":go_default_library"],
deps = [
"//prow/config:go_default_library",
Expand Down
145 changes: 145 additions & 0 deletions prow/tide/contextregister.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
/*
Copyright 2018 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package tide

import (
"sync"

"github.com/shurcooL/githubql"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/test-infra/prow/config"
)

type contextChecker interface {
// ignoreContext tells whether a context is optional.
ignoreContext(context Context) bool
// missingRequiredContexts tells if required contexts are missing from the list of contexts provided.
missingRequiredContexts([]Context) []Context
}

// newExpectedContext creates a Context with Expected state.
// This should not only be used when contexts are missing.
func newExpectedContext(c string) Context {
return Context{
Context: githubql.String(c),
State: githubql.StatusStateExpected,
Description: githubql.String(""),
}
}

// contextRegister implements contextChecker and allow registering of required and optional contexts.
type contextRegister struct {
lock sync.RWMutex
required, optional sets.String
skipUnknownContexts bool
}

// newContextRegister instantiates a new contextRegister and register tide as optional by default
// and uses Prow Config to find optional and required tests, as well as skipUnknownContexts.
func newContextRegister(skipUnknownContexts bool) *contextRegister {
r := &contextRegister{
required: sets.NewString(),
optional: sets.NewString(),
skipUnknownContexts: skipUnknownContexts,
}
r.registerOptionalContexts(statusContext)
return r
}

// newContextRegisterFromPolicy instantiates a new contextRegister and register tide as optional by default
// and uses Prow Config to find optional and required tests, as well as skipUnknownContexts.
func newContextRegisterFromPolicy(policy *config.Policy) *contextRegister {
r := newContextRegister(false)
if policy != nil {
if policy.SkipUnknownContexts != nil {
r.skipUnknownContexts = *policy.SkipUnknownContexts
}
if policy.Protect != nil && *policy.Protect {
r.registerRequiredContexts(policy.Contexts...)
} else if r.skipUnknownContexts {
r.registerRequiredContexts(policy.Contexts...)
}

}
return r
}

// newContextRegisterFromPolicy instantiates a new contextRegister and register tide as optional by default
// and uses Prow Config to find optional and required tests, as well as skipUnknownContexts.
func newContextRegisterFromConfig(org, repo, branch string, c *config.Config) (*contextRegister, error) {
policy, err := c.GetBranchProtection(org, repo, branch)
if err != nil {
return nil, err
}
_, optional := config.BranchRequirements(org, repo, branch, c.Presubmits)
r := newContextRegisterFromPolicy(policy)
r.registerOptionalContexts(optional...)
return r, nil
}

// ignoreContext checks whether a context can be ignored.
// Will return true if
// - context is registered as optional
// - required contexts are registered and the context provided is not required
// Will return false otherwise. Every context is required.
func (r *contextRegister) ignoreContext(c Context) bool {
Copy link
Member

Choose a reason for hiding this comment

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

This function need to use the mutex as a reader.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

r.lock.RLock()
defer r.lock.RUnlock()
if r.optional.Has(string(c.Context)) {
return true
}
if r.required.Has(string(c.Context)) {
return false
}
if r.skipUnknownContexts {
return true
}
return false
}

// missingRequiredContexts discard the optional contexts and only look of extra required contexts that are not provided.
func (r *contextRegister) missingRequiredContexts(contexts []Context) []Context {
Copy link
Member

Choose a reason for hiding this comment

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

This function need to use the mutex as a reader.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

r.lock.RLock()
defer r.lock.RUnlock()
if r.required.Len() == 0 {
return nil
}
existingContexts := sets.NewString()
for _, c := range contexts {
existingContexts.Insert(string(c.Context))
}
var missingContexts []Context
for c := range r.required.Difference(existingContexts) {
missingContexts = append(missingContexts, newExpectedContext(c))
}
return missingContexts
}

// registerOptionalContexts registers optional contexts
func (r *contextRegister) registerOptionalContexts(c ...string) {
r.lock.Lock()
defer r.lock.Unlock()
r.optional.Insert(c...)
}

// registerRequiredContexts register required contexts.
// Once required contexts are registered other contexts will be considered optional.
func (r *contextRegister) registerRequiredContexts(c ...string) {
r.lock.Lock()
defer r.lock.Unlock()
r.required.Insert(c...)
}
Loading