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
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 required checks are passing, meaning we will skip optional contexts.
Copy link
Contributor

Choose a reason for hiding this comment

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

So if I understand this correctly, branch protection configuration stored in github specifies required contexts. Also optional in the presubmits specifies required contexts from the prow side. Is tide going to merge based on the union of these contexts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So instead of using the data stored in github (querying github for required check on every PR consumes too many token), we use the branch protection data stored in the config. The branch protection parse the config + the prow jobs settings and all that information goes into the contextRegister


Example:

```yaml
tide:
...
skip_optional_contexts: true
Copy link
Contributor

Choose a reason for hiding this comment

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

This now lives in the TideQuery struct above, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. I tried this option, but it does not feel natural (you can't really query it) and is a bit hacky I feel. There could be multiple tide queries per PR, subpools regroups PR per (org, repo, branch), not per Tide query, so that means that we need to attach the query or the information to the PullRequest struct since querying happens way before we check for required checks. Right now I simplified this PR by creating a global tide option, but I think the right place to put this would be in the branch protection configuration. I can do that in another PR.

```
8 changes: 8 additions & 0 deletions prow/config/jobs.go
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,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
4 changes: 4 additions & 0 deletions prow/config/tide.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@ type Tide struct {
// controller to handle org/repo:branch pools. Defaults to 20. Needs to be a
// positive number.
MaxGoroutines int `json:"max_goroutines,omitempty"`

// SkipOptionalContexts will use BranchProtection configuration options to know
// which contexts can be skipped in order to merge a PR.
SkipOptionalContexts bool `json:"skip_optional_contexts,omitempty"`
}

// MergeMethod returns the merge method to use for a repo. The default of merge is
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
107 changes: 107 additions & 0 deletions prow/tide/contextregister.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
/*
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"
)

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
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the need for thread safety... I don't see where we concurrently register required contexts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see why it would hurt anything. Tide runs multiple things in goroutine.

Copy link
Member

Choose a reason for hiding this comment

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

Is this lock actually necessary? It doesn't look like the contextRegister is ever used concurrently.
If it is necessary, please either switch it to a normal mutex or use the RLock and RUnlock methods where appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand this comment ? Why having a lock would hurt ? if there no concurrency it does not change perf and if there is it does the right thing. What is the technical concern that I am missing ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to read lock.

Copy link
Member

Choose a reason for hiding this comment

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

I was just wondering why it was added since this doesn't seem like something that would require locking given that we need to register all the contexts before it can be used and I would expect that to always be done from one thread.

It doesn't hurt though and is definitely fine to leave as is! We may encounter a concurrent use case later.

required, optional sets.String
}

// newContextRegister instantiates a new contextRegister and register the optional contexts provided.
func newContextRegister(optional ...string) *contextRegister {
r := contextRegister{
required: sets.NewString(),
optional: sets.NewString(),
}
r.registerOptionalContexts(optional...)
return &r
}

// 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.Lock()
defer r.lock.Unlock()
if r.optional.Has(string(c.Context)) {
return true
}
if r.required.Len() > 0 && !r.required.Has(string(c.Context)) {
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.Lock()
defer r.lock.Unlock()
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...)
}
131 changes: 131 additions & 0 deletions prow/tide/contextregister_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
/*
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 (
"testing"

"k8s.io/apimachinery/pkg/util/sets"
)

func TestContextRegisterIgnoreContext(t *testing.T) {
testCases := []struct {
name string
required, optional []string
contexts []string
results []bool
}{
{
name: "only optional contexts registered",
contexts: []string{"c1", "o1", "o2"},
optional: []string{"o1", "o2"},
results: []bool{false, true, true},
},
{
name: "no contexts registered",
contexts: []string{"t2"},
results: []bool{false},
},
{
name: "only required contexts registered",
required: []string{"c1", "c2", "c3"},
contexts: []string{"c1", "c2", "c3", "t1"},
results: []bool{false, false, false, true},
},
{
name: "optional and required contexts registered",
optional: []string{"o1", "o2"},
required: []string{"c1", "c2", "c3"},
contexts: []string{"o1", "o2", "c1", "c2", "c3", "t1"},
results: []bool{true, true, false, false, false, true},
},
}

for _, tc := range testCases {
cr := newContextRegister(tc.optional...)
cr.registerRequiredContexts(tc.required...)
for i, c := range tc.contexts {
if cr.ignoreContext(newExpectedContext(c)) != tc.results[i] {
t.Errorf("%s - ignoreContext for %s should return %t", tc.name, c, tc.results[i])
}
}
}
}

func contextsToSet(contexts []Context) sets.String {
s := sets.NewString()
for _, c := range contexts {
s.Insert(string(c.Context))
}
return s
}

func TestContextRegisterMissingContexts(t *testing.T) {
testCases := []struct {
name string
required, optional []string
existingContexts, expectedContexts []string
}{
{
name: "no contexts registered",
existingContexts: []string{"c1", "c2"},
},
{
name: "optional contexts registered / no missing contexts",
optional: []string{"o1", "o2", "o3"},
existingContexts: []string{"c1", "c2"},
},
{
name: "required contexts registered / missing contexts",
required: []string{"c1", "c2", "c3"},
existingContexts: []string{"c1", "c2"},
expectedContexts: []string{"c3"},
},
{
name: "required contexts registered / no missing contexts",
required: []string{"c1", "c2", "c3"},
existingContexts: []string{"c1", "c2", "c3"},
},
{
name: "optional and required contexts registered / missing contexts",
optional: []string{"o1", "o2", "o3"},
required: []string{"c1", "c2", "c3"},
existingContexts: []string{"c1", "c2"},
expectedContexts: []string{"c3"},
},
{
name: "optional and required contexts registered / no missing contexts",
optional: []string{"o1", "o2", "o3"},
required: []string{"c1", "c2"},
existingContexts: []string{"c1", "c2", "c4"},
},
}

for _, tc := range testCases {
cr := newContextRegister(tc.optional...)
cr.registerRequiredContexts(tc.required...)
var contexts []Context
for _, c := range tc.existingContexts {
contexts = append(contexts, newExpectedContext(c))
}
missingContexts := cr.missingRequiredContexts(contexts)
m := contextsToSet(missingContexts)
if !m.Equal(sets.NewString(tc.expectedContexts...)) {
t.Errorf("%s - expected %v got %v", tc.name, tc.expectedContexts, missingContexts)
}
}
}
Loading