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

sebastienvas · 2018-03-28T17:48:24Z

Ref: https://goo.gl/cd9MEu which was discussed with @fejta @cjwagner @BenTheElder

Instead of getting required checks from the github api, it will use the branch protection configuration and infere the required context from it. We are also adding a optional options to Prow Presubmits.

Config would look like this:

...
branch-protection:
  skip-unknown-contexts: true

...
presubmits:
  org/repo:
  - name: my-presubmit
    optional: true
...

#7140

0xmichalis · 2018-03-28T20:12:30Z

prow/github/client.go

@@ -930,6 +930,19 @@ func (c *Client) GetCombinedStatus(org, repo, ref string) (*CombinedStatus, erro
 	return &combinedStatus, err
 }

+// GetRequiredChecks returns the latest statuses for a given ref.
+func (c *Client) GetRequiredChecks(org, repo, ref string) ([]string, error) {
+	c.log("GetCombinedStatus", org, repo, ref)


GetRequiredChecks

0xmichalis · 2018-03-28T20:13:11Z

prow/config/tide.go

@@ -78,6 +83,61 @@ func (t *Tide) MergeMethod(org, repo string) github.PullRequestMergeType {
 	return v
 }

+// TideBranchProtection is initially used to know whether to use the github status or only check for required checks.
+// This can extended in the future to support


unfinished sentence

How do I configure the required checks? If this is something that is done on Github alongside configuring branch protection, please add pointers to documentation on how to do it.

cjwagner · 2018-03-29T00:16:48Z

prow/config/tide.go

+		return true
+	}
+	for _, b := range o.Branches {
+		if strings.Compare(b, branch) == 0 {


You can just use == to check for string equality in golang.

cjwagner · 2018-03-29T00:17:33Z

prow/config/tide_test.go

+			expected: TideBranchProtection{UseRequiredCheckOnly: true},
+		},
+		{
+			name: "org, repo and branch listed",


All the tests have the same name.

cjwagner · 2018-03-29T00:19:12Z

prow/github/client.go

@@ -930,6 +930,19 @@ func (c *Client) GetCombinedStatus(org, repo, ref string) (*CombinedStatus, erro
 	return &combinedStatus, err
 }

+// GetRequiredChecks returns the latest statuses for a given ref.


This needs to be updated.

cjwagner · 2018-03-29T00:29:17Z

prow/tide/tide.go

+
+	requiredChecksSet := sets.NewString()
+	if bp.UseRequiredCheckOnly {
+		requiredChecks, err := ghc.GetRequiredChecks(


This could use too many API tokens. We need to cache this info, but I'm not sure what a sane eviction policy would be. Maybe just use a 1 hour TTL?
A proxy cache layer would handle this transparently. @stevekuznetsov @BenTheElder

I don't really like the idea that we'll pull this from GitHub and lag behind the current state or that we'll instead eat API tokens for this. We could accomplish the same thing by instead requiring you to set the list of required statuses in Prow when you set them on GitHub.

It's more effort and but it's also much less complex and less magical. This worked fine for the submit-queue...

With the branch protector we're also currently deriving these from Prow config for Kubernetes repos anyhow, that would just leave non-Prow statuses, which generally seem to be made required without automation anyhow.

cjwagner · 2018-03-29T00:43:15Z

prow/tide/tide.go

+		}
+		requiredChecksSet.Insert(requiredChecks...)
+	}
+	combinedSet := sets.NewString()


I think this variable name is a misnomer. It is the set of contexts seen on the PR, nothing is combined?

cjwagner · 2018-03-29T00:47:37Z

prow/tide/tide.go

 			return false
 		}
 	}
+	diff := requiredChecksSet.Difference(combinedSet)
+	if diff.Len() != 0 {
+		log.Info("the following checks are missing %v", strings.Join(diff.List(), ". "))


s/". "/", "/
s/Info/Warnf/

cjwagner · 2018-03-29T00:51:24Z

prow/tide/tide.go

@@ -678,7 +701,8 @@ func (c *Controller) pickBatch(sp subpool) ([]PullRequest, error) {
 		if i == 5 {
 			break
 		}
-		if !isPassingTests(sp.log, c.ghc, pr) {
+		bp := c.ca.Config().Tide.BranchProtection.GetBranchProtection(sp.org, sp.repo, sp.branch)
+		if !isPassingTests(sp.log, c.ghc, pr, bp) {


pickBatch should accept bp as a parameter from its caller instead of looking it up from the config.

stevekuznetsov · 2018-03-29T20:23:19Z

This should be paired with a change to the PR dashboard as well

0xmichalis · 2018-03-29T20:52:09Z

prow/config/tide.go

@@ -78,6 +83,61 @@ func (t *Tide) MergeMethod(org, repo string) github.PullRequestMergeType {
 	return v
 }

+// TideBranchProtection is initially used to know whether to use the github status or only check for required checks.
+// This can extended in the future to support all the branch protection options.


Needs a link to github docs for setting up the required checks.

Do you have an example ?

I added a README.md file in cmd/tide

sebastienvas · 2018-03-30T04:09:43Z

@stevekuznetsov I don't understand what you mean. Can you elaborate ?

sebastienvas · 2018-03-30T07:20:27Z

@stevekuznetsov please take an extra look since this PR conflicted with #7454

stevekuznetsov

I'll create a deck PR to change the required branches and link you there to show an example of what I mean.

stevekuznetsov · 2018-04-02T16:15:39Z

prow/tide/tide.go

@@ -157,6 +163,24 @@ func NewController(ghcSync, ghcStatus *github.Client, kc *kube.Client, ca *confi
 	}
 }

+func (b *branchProtection) skipContext(c string) bool {


Maybe contextRequired as a name?

stevekuznetsov · 2018-04-02T16:16:20Z

prow/tide/tide.go

+	}
+	var ctxs []Context
+	for _, c := range b.requiredChecks.Difference(c).List() {
+		ctxs = append(ctxs, Context{Context: githubql.String(c)})


Shouldn't construct semi-initialized Context object here, someone's going to nil pointer downstream of this

stevekuznetsov · 2018-04-02T16:17:53Z

prow/tide/tide.go

 	for _, ctx := range contexts {
+		combinedSet.Insert(string(ctx.Context))


Let's break this out -- the doc on this method reads "unsuccessfulContexts determines which contexts from the list are failed" -- contexts that are not in the list should not be returned? Maybe a method missingContexts -- but unsure of the usefulness of this actually as once you mark a context required, GitHub automatically adds it to every commit, so we're coding against GitHub acting up?

Or actually on second thought, you are changing the semantics of the check entirely -- rename this method to reflect the fact that instead of identifying failed contexts from an owning set, you are checking for successful contexts from a required set.

I don't think we are changing the semantic. Here, we still skip the test we don't care about, ie tests that are not required, and we add required tests that are missing. (In theory this should not happen, as github will add them as pending by default, but it is a safe guard). I updated the documentation.

stevekuznetsov · 2018-04-02T16:19:13Z

prow/tide/tide.go

+		config: c.ca.Config().Tide.BranchProtection.GetBranchProtection(sp.org, sp.repo, sp.branch),
+	}
+	if bp.config.UseRequiredChecksOnly {
+		requiredChecks, err := c.ghc.GetRequiredChecks(sp.org, sp.repo, sp.branch)


Why should we fetch this twice (in takeAction and here) for the same controller?

As stated above, we shouldn't be fetching this from Github at all. Rather, we should maintain a list of contexts that can be ignored in the Prow config.

Yes, right now AFAIK all users are either:

manually setting required jobs, in which case it shouldn't be a big deal to add them to a prow config entry

autogenerating the required jobs from prow config with the branch protector, in which case we should reuse that.

I think this will have much less surprising behavior than attempting to sync with github behind a cache. Without some form of caching this will probably waste too many API calls.

we should not :)

Why can't a required context be inferred by the following combination in the config:

job exists (thus can be controlled)

job.always_run: true || job.run_if_changed != ""

job.skip_report: false

@Kargakis this only works for Prow jobs, what about other CI ?

I tought of getting the required contexts from the PR using githubql as this would not take any additional api call, but then I saw you were already making an API call for combined status, so the same argument holds.

No? Getting the required contexts does cost more.

it will be out of sync, because that s problem that we have right now with mungegithub. We have multiple admins, and people doing changes out of our control. And you can tell people that they need to update it, but you cannot force them to.

Not if you write it in a config in the first place, which also creates a public record and controls it via PR instead of admins randomly changing merge requirements silently. Nothing is out of sync if we push out the requirements to GitHub on config load instead of trying to keep up with GitHub.

And you can tell people that they need to update it, but you cannot force them to.

Well I mean, you can't force them to use tide or the queue either, they can click merge manually even when it is deployed, but we don't optimize for this sort of broken usage either...

headContexts should not be used as an example, it is a hacky workaround for getting the status contexts for the most recent commit even though Github returns the commits in a non-useful ordering. I just had a discussion with @BenTheElder about more scalable alternatives to this.
Furthermore, headContexts uses far fewer tokens than fetching the required contexts like you are doing would. headContexts doesn't usually have to actually make an API call whereas GetRequiredChecks always makes an API call.

Using GetRequiredChecks the way it is used now is prohibitively expensive. Tide is designed to operate on multiple thousands of open PRs. If we listed the required contexts for each of these PRs when setting statuses, Tide's status sync loop could use up all of the bots hourly API tokens in a matter of minutes all on its own. This is problematic because the tokens are shared by Tide's main sync loop and all of the other Prow components.

That's fair, but it disabled by default in your use case. So can we still get this in until we have a better approach ?

stevekuznetsov · 2018-04-02T16:20:27Z

prow/tide/tide.go

@@ -987,6 +1054,7 @@ type PullRequest struct {
 	}
 	HeadRefOID githubql.String `graphql:"headRefOid"`
 	Mergeable  githubql.MergeableState
+


nit: You've got a couple of these newline diffs, maybe remove?

stevekuznetsov · 2018-04-02T16:22:42Z

prow/tide/tide_test.go

@@ -1233,7 +1248,7 @@ func TestHeadContexts(t *testing.T) {

 	for _, tc := range testCases {
 		t.Logf("Running test case %q", tc.name)
-		fgc := &fgc{}
+		fgc := &fgc{combinedStatus: map[string]string{win: "SUCCESS"}}


Do we want to use the GithubQL types instead of raw string literals for the context status?

That would be changing the signature of the interface method.

but I used the githubql const.

stevekuznetsov · 2018-04-02T16:23:14Z

prow/tide/tide_test.go

+
+func TestIsPassing(t *testing.T) {
+	headSHA := "head"
+	success := "SUCCESS"


githubql.StatusStateSuccess ?

stevekuznetsov · 2018-04-02T16:23:26Z

prow/tide/tide_test.go

+func TestIsPassing(t *testing.T) {
+	headSHA := "head"
+	success := "SUCCESS"
+	failure := "FAILED"


githubql.StatusStateFailure?

stevekuznetsov · 2018-04-02T16:24:18Z

prow/tide/tide_test.go

+		requiredContexts      []string
+	}{
+		{
+			name:                  "required checks but not used - passing",


"required checks but not used" what does this mean? Maybe "no required checks configured"?

Wait, why is this a valid case? Shouldn't we never query GitHub for the list of required checks if the useRequiredChecksOnly field is not true?

Right this is what the test is checking, I renamed it. PTAL

stevekuznetsov · 2018-04-02T16:26:49Z

prow/tide/tide_test.go

+			skipContext: true,
+		},
+		{
+			name: "config no required checks",


This seems like something we should validate at config load time -- if you require checks but configured none, that's a user error in config

the required checks are coming from the github api, so validating the config would require an api call for every branch, plus checking it at config parsing does not actually guarantees that it'll still be valid a couple of minute after.

I am doing validation now since I am gertting the required checks from the branch protection config.

sebastienvas · 2018-04-02T21:34:19Z

Thanks for the review @stevekuznetsov PTAL

cjwagner

I think using a whitelist/blacklist approach is the right way to go in order to save API tokens and keep config in one place instead of spread across both Prow and Github.
I'm fine with this merging as a temporary solution if this is needed urgently as long as the intent is to change it soon. What do others think?

cjwagner · 2018-04-03T23:22:45Z

prow/tide/tide.go

+		return nil
+	}
+	var ctxs []Context
+	for _, c := range b.requiredChecks.Difference(c).List() {


Please avoid variable shadowing (c).

sebastienvas · 2018-04-10T23:53:38Z

@fejta @cjwagner @BenTheElder @stevekuznetsov could we continue the review ? I'll resolved the conflicts once we have an agreement.

stevekuznetsov · 2018-04-11T15:00:19Z

Whitelist/blacklist seems like a good approach but what were the original design intents behind tide? IIRC they were to use the query and just have an overall passing check, right? It was not even to post a status and just use the query API for aggregate status. Why did we want that? The more we head down this rabbit hole the more we lose whatever we were after with that. FWIW I think it's inevitable because of the reality of GitHub status contexts, but we should just keep that in mind...

sebastienvas · 2018-04-11T22:19:11Z

/assign fejta

fejta · 2018-04-11T22:20:38Z

prow/cmd/branchprotector/protect.go

@@ -39,11 +38,11 @@ type options struct {

 func (o *options) Validate() error {
 	if o.config == "" {
-		return errors.New("empty --config")
+		return fmt.Errorf("empty --config")


Unnecessary change? It doesn't format anything.

it is just that it was inconsistent all over the code. Some use fmt.Errorf some use errors.New. Since errors was conflicting with errors renamed from Errors I used fmt.Errorf

will revert to ease review

fejta · 2018-04-11T23:13:05Z

prow/cmd/branchprotector/protect.go

-		if !after && !j.AlwaysRun && j.RunIfChanged == "" {
-			continue // No
-		}
-		if !j.SkipReport { // This job needs a context


One thing we need is to change this to if !j.SkipReport && !j.Optional

what is optional ?

fejta · 2018-04-11T23:15:02Z

prow/cmd/branchprotector/protect.go

-	return required
-}
-
-func repoRequirements(org, repo string, cfg config.Config) []string {


A second thing would be to write a allBranchRequirements(org, repo, branch string, cfg config.Confg) that combines repoRequirements() with the union of all the contexts at branch, repo, org, overall levels...

For this PR can we focus on making these minimal set of changes and save refactoring the whole thing for later?

that s done here https://sourcegraph.com/github.com/sebastienvas/k8s-test-infra@34a25d3febb106ca88b5ba7c8560f73cf791609f/-/blob/prow/config/branch_protection.go#L121

I moved those to the branch_protection config as this is config parsing

fejta · 2018-04-11T23:18:12Z

prow/config/jobs.go

@@ -101,6 +101,8 @@ type Presubmit struct {
 	Spec *v1.PodSpec `json:"spec,omitempty"`
 	// Run these jobs after successfully running this one.
 	RunAfterSuccess []Presubmit `json:"run_after_success"`
+	// Defines if a test is a required check as defined in the Github Branch protection.
+	NonRequiredCheck bool `json:"non_required_check,omitempty"`


Please avoid double negatives.

if optional is easier on the brain than if !NonRequiredCheck

well I have to make nonrequired default so I don't have much choice.

Choose a better name than non-required, look up antonyms of required if you don't like my naming suggestion (optional).

optional = false is easier to understand than nonrequired = false

you are right ! Yes optional is better, sorry somehow I understood that optional was reverse of non required. I ll change it.

fejta · 2018-04-11T23:21:12Z

prow/config/tide.go

+	// BranchProtection specifies branch protection options.
+	// Initially it is used for allowing to merge a PR when required checks only are passing,
+	// but it can be extended with more of the github features.
+	BranchProtection TideProtection `json:"branchProtection,omitempty"`


I don't understand why this is necessary. Tide can just read the branch-protection options?

don't you think one would still want to prevent merging when one of the non required checks is failing ?If not then, you are right.

@BenTheElder @cjwagner what do you think ? I am fine either way.

This makes more sense as an enum/boolean field on each TideQuery

Just like I can toss a reviewApprovedRequired: true into a query I'd expect to be able to toss a ignoreOptionalContexts: true here

I was gonna do that first, but then since you can't really query it I decided to use a separate option, but I really like your idea.

actually looking at the implementation, you can define multiple queries per PR, and therefore you could define a query with ignoreOptionalContexts enabled and another disabled which could be a nice option, however when we go in the subpool we loose the query information. We could hack the code to make this happen, but that's ugly. The more I think about it, the more I think we should have an option in the branch protection config. What do you think ?

fejta · 2018-04-11T23:23:11Z

prow/tide/tide.go

@@ -200,7 +200,7 @@ func byRepoAndNumber(prs []PullRequest) map[string]PullRequest {
 // Note: an empty diff can be returned if the reason that the PR does not match
 // the TideQuery is unknown. This can happen happen if this function's logic
 // does not match GitHub's and does not indicate that the PR matches the query.
-func requirementDiff(pr *PullRequest, q *config.TideQuery) (string, int) {
+func requirementDiff(pr *PullRequest, q *config.TideQuery, bp config.TideBranchProtection) (string, int) {


I think it would be useful to better separate out the concerns here, so send optionalContexts sets.String or else an interface like:

type foo interface { requiredSubset([]string) []string }

I ll wait for you to answer to "don't you think one would still want to prevent merging when one of the non required checks is failing ?If not then, you are right." before fixing this one

fejta · 2018-04-11T23:36:55Z

prow/cmd/branchprotector/protect.go

-		// Unopinionated org, just set explicitly defined repos
-		for r := range org.Repos {
-			repos = append(repos, r)
+	// Do not automatically protect tested repositories


Please do not change default behavior in this PR

I am not this is coming from https://sourcegraph.com/github.com/kubernetes/test-infra@f2213eb3e19d95930c192db44ac06dffc1f4caf9/-/blob/prow/cmd/branchprotector/protect.go#L198

Note that yours tests were pretty good, so it was easy to make sure that my change kept the funcionality

Given that I'm essentially in the middle of refactoring this whole file to support the rest of the github options, a secondary goal is to make it as easy as possible for me to understand what parts you need me to pull in

fejta · 2018-04-11T23:38:35Z

prow/cmd/branchprotector/protect.go

-	Protect  bool
-	Contexts []string
-	Pushers  []string
+type requirements struct {


I agree with this sort of thing generally (I had done something similar in my PR), but given that there is already a lot going on in this PR please keep as many lines the same as possible (I removed those changes from my PR to help simplify review).

ok will fix

0xmichalis · 2018-04-12T07:46:00Z

This PR is too large to review and needs to be broken down into smaller self-contained PRs. Before that happens I would suggest writting up a proposal in a google doc where we can bikeshed the design of this and discuss alternatives.

k8s-ci-robot · 2018-04-12T18:59:19Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sebastienvas
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: fejta

Assign the PR to them by writing /assign @fejta in a comment when ready.

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:

prow/OWNERS

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

fejta

Can we squash/rebase away some of these commits?

0xmichalis · 2018-04-18T08:46:44Z

prow/tide/contextregister.go

+}
+
+func (ic tideIC) ignoreContext(c string) bool {
+	if c == statusContext {


nit: return c == statusContext

0xmichalis · 2018-04-18T08:47:08Z

prow/tide/contextregister.go

+	r.ics = append(r.ics, i)
+}
+
+func (ic tideIC) ignoreContext(c string) bool {


Add a comment that this is used for ignoring the tide status context.

I actually changed the implementation to be a lot simpler.

0xmichalis · 2018-04-18T08:54:30Z

prow/cmd/tide/README.md

+
+### 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.


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?

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

fejta

@cjwagner @Kargakis @stevekuznetsov can one of you take the lead for this PR?

This feels more like your part of the codebase, want to make sure it is written according to your preferred style &c

fejta · 2018-04-19T01:24:28Z

prow/cmd/tide/README.md

+```yaml
+tide:
+  ...
+  skip_optional_contexts: true


This now lives in the TideQuery struct above, right?

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.

fejta · 2018-04-19T01:26:48Z

prow/tide/contextregister.go

+	"k8s.io/apimachinery/pkg/util/sets"
+)
+
+type ContextChecker interface {


what other packages need a ContextChecker?

fejta · 2018-04-19T05:20:22Z

prow/tide/contextregister.go

+
+// ContextRegister implements ContextChecker and allow registering of required and optional contexts.
+type ContextRegister struct {
+	lock               sync.RWMutex


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

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

fejta · 2018-04-19T05:22:40Z

prow/tide/contextregister.go

+}
+
+// NewContextRegister instantiates a new ContextRegister and register the optional contexts provided.
+func NewContextRegister(optional ...string) *ContextRegister {


I don't see other packages using this method. Can we keep things private until they are needed?

Everthing should be private I guess.

sebastienvas · 2018-04-20T16:36:03Z

@stevekuznetsov @Kargakis @cjwagner could you please take a final look. Based on all the feedback I reduced the change dramatically to ease code review.

cjwagner · 2018-04-23T21:03:07Z

prow/tide/contextregister.go

+// - 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 {


This function need to use the mutex as a reader.

cjwagner · 2018-04-23T21:03:33Z

prow/tide/contextregister.go

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


This function need to use the mutex as a reader.

cjwagner · 2018-04-23T21:18:34Z

prow/tide/tide.go

+				log.WithError(err).Error("Getting branch protection")
+				return
+			}
+			cr.registerRequiredContexts(findRequiredContexts(bp)...)


It doesn't seem right that all of this is conditional on sc.ca.Config().Tide.SkipOptionalContexts.
If SkipOptionalContexts is false we should still require the required contexts.
Additionally, it doesn't look like we are ever actually adding the optional contexts with registerOptionalContexts.

Should we get ride of the SkipOptionalContexts all together ?

The way it is implemented now, is that once required checks are registered all other checks are optional, so you don't need to register the optional ones.

It doesn't provide all of the behavior cases that we need to support. I think what we really want is a SkipUnknownContexts instead.
As this PR is currently, if we require that all statuses are green to merge (including ephemeral ones that might be on a PR) by setting SkipOptionalContexts=false we wouldn't be able to require/expect specific statuses. This is problematic because this is the default value for a Tide config and will likely be the common case for smaller deployments since requiring all statuses to be green to merge is simple and intuitive.

We should change the contextRegister to have a set of required contexts, a set of known optional contexts (tide and jobs marked optional) and a bool indicating if unknown statuses should be required (SkipUnknownContexts).

When we are checking if a context can be ignored we just check that it is either

in the optional set
or

it is not in the required set and SkipUnknownContexts is true.

When we are checking for missing required contexts we just ensure that all the contexts in the required set are present.

WDYT?

I think there is a misunderstanding, if SkipOptionalContexts=false it is exactly as it is today. the only optional check will be tide and all other will be considered required.

https://sourcegraph.com/github.com/sebastienvas/k8s-test-infra@7b4ef04aa5779be59c91d555fb697f8339d81c7f/-/blob/prow/tide/contextregister.go#L70

The problem is that if SkipOptionalContexts=false we don't want to just require that all statuses we see are green. We also want to ensure that all the required statuses actually appear and are not missing.
In other words the ignoreContext function will work properly when SkipOptionalContexts=false, but the missingRequiredContexts function will not because no required contexts will have been registered.

which is not different than today

cjwagner · 2018-04-23T21:20:50Z

prow/tide/tide.go

@@ -972,11 +993,18 @@ func (c *Controller) presubmitsByPull(sp subpool) (map[int]sets.String, error) {
 }

 func (c *Controller) syncSubpool(sp subpool) (Pool, error) {
-	sp.log.Infof("Syncing subpool: %d PRs, %d PJs.", len(sp.prs), len(sp.pjs))


Please leave this log message.

cjwagner · 2018-04-23T21:22:42Z

prow/tide/tide.go

+		if err != nil {
+			return Pool{}, fmt.Errorf("error parsing branch protection: %v", err)
+		}
+		cr.registerRequiredContexts(findRequiredContexts(bp)...)


Same comment as above in setStatuses.

cjwagner · 2018-04-23T21:25:58Z

prow/tide/tide_test.go

+		config           *config.Branch
+		combinedContexts map[string]string
+	}{
+		{


Please add some test cases for optional contexts.

cjwagner · 2018-04-24T21:41:26Z

prow/tide/contextregister.go

+
+// contextRegister implements contextChecker and allow registering of required and optional contexts.
+type contextRegister struct {
+	lock               sync.RWMutex


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.

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 ?

Updated to read lock.

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.

cjwagner · 2018-04-24T22:12:15Z

prow/tide/tide.go

+				log.WithError(err).Error("Getting branch protection")
+				return
+			}
+			cr.registerRequiredContexts(findRequiredContexts(bp)...)


It doesn't provide all of the behavior cases that we need to support. I think what we really want is a SkipUnknownContexts instead.
As this PR is currently, if we require that all statuses are green to merge (including ephemeral ones that might be on a PR) by setting SkipOptionalContexts=false we wouldn't be able to require/expect specific statuses. This is problematic because this is the default value for a Tide config and will likely be the common case for smaller deployments since requiring all statuses to be green to merge is simple and intuitive.

We should change the contextRegister to have a set of required contexts, a set of known optional contexts (tide and jobs marked optional) and a bool indicating if unknown statuses should be required (SkipUnknownContexts).

When we are checking if a context can be ignored we just check that it is either

in the optional set
or

it is not in the required set and SkipUnknownContexts is true.

When we are checking for missing required contexts we just ensure that all the contexts in the required set are present.

WDYT?

sebastienvas · 2018-04-26T16:33:32Z

PTAL

…o required-tests

sebastienvas · 2018-05-08T00:04:54Z

@cjwagner Please take a look!

sebastienvas · 2018-05-09T20:57:25Z

closed in favor of #7983

sebastienvas requested review from cjwagner and stevekuznetsov as code owners March 28, 2018 17:48

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 28, 2018

0xmichalis reviewed Mar 28, 2018

View reviewed changes

cjwagner reviewed Mar 29, 2018

View reviewed changes

0xmichalis reviewed Mar 29, 2018

View reviewed changes

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 30, 2018

stevekuznetsov requested changes Apr 2, 2018

View reviewed changes

sebastienvas mentioned this pull request Apr 3, 2018

Migrate mungegithub to kubernetes-retired #6104

Closed

cjwagner reviewed Apr 3, 2018

View reviewed changes

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 8, 2018

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 11, 2018

k8s-ci-robot assigned fejta Apr 11, 2018

fejta reviewed Apr 11, 2018

View reviewed changes

sebastienvas force-pushed the required-tests branch from 2264e4b to 71da3d1 Compare April 12, 2018 19:54

fejta reviewed Apr 18, 2018

View reviewed changes

0xmichalis reviewed Apr 18, 2018

View reviewed changes

sebastienvas force-pushed the required-tests branch 3 times, most recently from 63d1e7c to c755b01 Compare April 18, 2018 23:54

Use branch protection setting in tide

c233bf0

sebastienvas force-pushed the required-tests branch from c755b01 to c233bf0 Compare April 19, 2018 03:06

fejta reviewed Apr 19, 2018

View reviewed changes

Make contextregister private

82e3879

cjwagner reviewed Apr 23, 2018

View reviewed changes

Fix comments

20b07e4

cjwagner reviewed Apr 24, 2018

View reviewed changes

Use Read lock when required

7b4ef04

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 28, 2018

0xmichalis mentioned this pull request May 4, 2018

document tide #5919

Closed

Merge branch 'master' of https://github.com/kubernetes/test-infra int…

7c6e5e4

…o required-tests

sebastienvas mentioned this pull request May 7, 2018

Adds simple documentation for tide #7955

Closed

Add SkipUnknownContexts to the branch protection config

cc09455

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 7, 2018

sebastienvas added 3 commits May 7, 2018 17:00

Fix configuration

fea785b

Update documentation

1016f6d

Fix linting

0a105df

sebastienvas mentioned this pull request May 9, 2018

Adds ability to set required and optional checks #7983

Merged

sebastienvas closed this May 9, 2018

		for _, ctx := range contexts {
		combinedSet.Insert(string(ctx.Context))

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

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

Conversation

sebastienvas commented Mar 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevekuznetsov commented Mar 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastienvas commented Mar 30, 2018

sebastienvas commented Mar 30, 2018

stevekuznetsov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastienvas Apr 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastienvas Apr 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastienvas commented Apr 2, 2018 • edited Loading

cjwagner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastienvas commented Apr 10, 2018

stevekuznetsov commented Apr 11, 2018

sebastienvas commented Apr 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastienvas commented Mar 28, 2018 •

edited

Loading

sebastienvas Apr 3, 2018 •

edited

Loading

sebastienvas Apr 2, 2018 •

edited

Loading

sebastienvas commented Apr 2, 2018 •

edited

Loading