Adds ability to set required and optional checks #7983

sebastienvas · 2018-05-09T20:56:48Z

Redoing #7457 as it takes forever to load.

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:

...
tide:
  ...
  context_options:
    policy:
      # Use branch protection options to define required and optional contexts
      from-branch-protection: true
      # Treat unknown contexts as optional
      skip-unknown-contexts: true
    orgs:
      org:
        repo:
          branch:
            policy:
              from-branch-protection: false
              required-contexts:
              - "required_test"
              optional-contexts:
              - "optional_test"

#7140

stevekuznetsov · 2018-05-09T21:09:03Z

/ok-to-test
/area prow
/kind feature

stevekuznetsov · 2018-05-09T21:11:10Z

prow/config/branch_protection.go

 	}
-	return required
+	return


-1 to named returns

Agreed on not using named returns

May I ask why ?

harder to maintain, it's not obvious what's the returned value

I find almost universally they are harder to reason about and read through. They are almost nonexistant in the codebase as it is. Stats:

$ grep -rh func prow/ | grep -Po "(?<=\) $)[^\($]+(?=\) \{)" | tr ',' '\n' | awk '{print $2 }' | grep -v "^$" | wc -l 19 $ grep -rh func prow/ | wc -l 2084

I actually discovered them in k8s codebase :) but right it may be confusing ! fixed.

stevekuznetsov · 2018-05-09T21:11:39Z

prow/config/branch_protection.go

 	p, ok := presubmits[org+"/"+repo]
 	if !ok {
-		return nil
+		return


-1 to named returns

stevekuznetsov · 2018-05-09T21:12:01Z

prow/config/branch_protection_test.go

@@ -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)


extend the test to test both?

stevekuznetsov · 2018-05-09T21:12:52Z

prow/cmd/branchprotector/README.md

@@ -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


Configuration for tide actions should not live in the branch protector's config.

+1, this should be part of the job spec

For prow jobs we already have the optional setting. For optional jobs that are not prowjob we need to define it somewhere. I had a global setting in tide configuration, but that was not practical. It makes sense for me to define it in branch protection since this option only works when branch protection is set and you might want to apply this globally, on a repo or on a branch which branch protection already does. I had discussed this with @cjwagner

also skipunknown context does not have any meaning if we don't have a branch protection, meaning no jobs defined at all in Prow config since all are unknown.

but that was not practical.

Why not? It's a simple mapping with the same parent/child scheme as in this config. I would expect it to live in tide config exclusively. It's configuring tide to run. Members can protect their branches without using the branch-protector and can run the branch-protector without running tide.

So the original idea was to use the github required check to figure that out, but that was doing too many request to the github api. After a meeting with the @fejta, @cjwagner and @BenTheElder it was decided to use branch protection instead. You don't have to actually enforce branch protection, you can just use the config and set protect to false, and then use that to define the required checks that you required for a PR to be merged. I don't see the point of duplicating those configs since they map back to protecting a branch. I think the Prow config is big and complicated enough.

You don't have to actually enforce branch protection, you can just use the config and set protect to false, and then use that to define the required checks that you required for a PR to be merged.

Err, what? If you want to configure branch protection for statuses, do that. Then let tide honor required statuses. If you want to configure statuses for tide, do that, and don't touch the branch protection config (which you might have never even written since you don't use the branch protector). IDK. These seem like two entirely separate use cases that are being put in one place.

I agree I am defining a tide options which will basically allow you to use branch protection or otherwise allow you to define your own if you don't want to use branch protection. If that option is not set, it will use combined status from github as it is today.

stevekuznetsov · 2018-05-09T21:13:16Z

prow/tide/contextregister.go

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


nit: indent of this repo should be in separate block at bottom

stevekuznetsov · 2018-05-09T21:29:26Z

prow/tide/tide.go

+			string(pr.BaseRef.Name),
+			sc.ca.Config())
+		if err != nil {
+			log.WithError(err).Error("Getting branch protection")


nit: bad error

stevekuznetsov · 2018-05-09T21:30:29Z

prow/tide/tide.go

 	var failed []Context
 	for _, ctx := range contexts {
-		if string(ctx.Context) == statusContext {
+		if cc.ignoreContext(ctx) {


this seems like a regression if we mark the tide status required

cc already ignores tide

stevekuznetsov · 2018-05-09T21:31:24Z

prow/tide/tide.go

@@ -944,7 +954,7 @@ func (c *Controller) presubmitsByPull(sp subpool) (map[int]sets.String, error) {
 	}()

 	for _, ps := range c.ca.Config().Presubmits[sp.org+"/"+sp.repo] {
-		if ps.SkipReport || !ps.RunsAgainstBranch(sp.branch) {
+		if !ps.ContextRequired() || !ps.RunsAgainstBranch(sp.branch) {


Why are we changing this functionality? If we want to do this, let's do it in a separate PR?

The goal of this PR is to allow optional jobs to be skipped for tide. So yes this is in scope.

Right now we have the SkipReport as the means by which we accomplish that. Why do we want to change that right now? Can we continue honoring the SkipReport as well? Breaking changes for cluster admins are annoying.

The reason is that SkipReport = false does not mean the check is required. In our use case you might want to push a status on a PR for a test that is not required, this is the reason I added Optional a while back. Optional is false by default so it will not impact anything, but then if you define a prowjob with Optional set to true, you defintely want to skip it to merge a PR. So this is the reason I added this.

I understand they are not identical features, what I am saying is today we use SkipReport to mean "tide ignore" because whether or not it's required, if it's not reported, tide ignores it. As long as SkipReport=true continues to mean that tide ignores the job, but now in addition, SkipReport=false Optional=true works, that is OK.

stevekuznetsov · 2018-05-09T21:33:11Z

prow/tide/tide_test.go

+		combinedContexts map[string]string
+	}{
+		{
+			name:    "required checks disabled should not impact - passing",


these names are a bit confusing

thanks for pointing that out. After reviewing some of the test I discovered some issues.

stevekuznetsov · 2018-05-09T21:33:33Z

prow/tide/tide_test.go

+		{
+			name:    "required checks disabled should not impact - passing",
+			passing: true,
+			config: &config.Policy{


@fejta is it even valid config to have a policy with explicitly listed required checks but have the policy be set to not enforce? why?

I think there could multiple use cases. When you define policy at the repo or org level, and then you create a branch for testing and you don't want to protect it or if you just want to disable temporarily.

sebastienvas · 2018-05-10T21:01:05Z

PTAL

stevekuznetsov · 2018-05-11T14:41:04Z

Please no merge commits. Rebase on master

sebastienvas · 2018-05-12T06:54:29Z

@stevekuznetsov I think we got it. I think you are going to like this.

sebastienvas

PTAL

sebastienvas · 2018-05-12T07:04:04Z

prow/tide/contextregister.go

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


I had it since tide code is using multiple go routine, but since the object is created inside a funciton, the lock is overkill.

sebastienvas · 2018-05-12T07:05:00Z

prow/tide/contextregister.go

+		optional:            sets.NewString(),
+		skipUnknownContexts: skipUnknownContexts,
+	}
+	r.registerOptionalContexts(statusContext)


it is coming from tide.go (its the tide status). I know the name is not very well chosen, but I am trying to do the least amount of changes.

sebastienvas · 2018-05-12T07:06:12Z

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 {


we do, that s tide status. its registered by default. This implementation could be use in another context, so I did not want to hard code in there.

sebastienvas · 2018-05-12T07:07:11Z

prow/tide/contextregister.go

+func (r *contextRegister) missingRequiredContexts(contexts []Context) []Context {
+	r.lock.RLock()
+	defer r.lock.RUnlock()
+	if r.required.Len() == 0 {


it will but why bother doing all that when you already know the answer.

cjwagner · 2018-05-15T18:17:00Z

prow/config/tide.go

+		return b
+	}
+	c := TideContextPolicy{}
+	c.FromBranchProtection = mergeBool(c.FromBranchProtection, b.FromBranchProtection)


Merge a and b, not c and b.

Please also add a test case to catch this.

cjwagner · 2018-05-15T18:29:38Z

prow/config/tide.go

+		if bp == nil {
+			return TideContextPolicy{}, errors.New("branch protection is not set")
+		}
+		if bp.Protect == nil || !*bp.Protect || bp.RequiredStatusChecks == nil {


I don't think that we should consider this an error. If I set FromBranchProtection at the org level I wouldn't expect an error if some repo in the org does not have RequiredStatusChecks. In this case we should probably just not add anything to required?

cjwagner · 2018-05-15T18:31:57Z

prow/config/tide.go

+		}
+		required.Insert(bp.RequiredStatusChecks.Contexts...)
+	}
+


Please add a TideContextPolicy.Validate function and call it here to validate the policy before it is returned.
Specifically, validate that no required context is also an optional context.

done and added tc

cjwagner · 2018-05-15T18:43:19Z

prow/tide/contextregister.go

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


I don't think that we need this any more. Its functionality could be moved into the TideContextPolicy struct.
In other words, instead of constructing a contextRegister from a TideContextPolicy in order to get a contextChecker, just make the TideContextPolicy match the contextChecker interface itself.

I tend to agree with you, but that would create circular dependency and I would have to move the Context struct out as an example. Plus I would have to hard code the tide status in there. I am trying to keep this as the minimum amount of change. However, I don't think that the context checker interface is something that we need. The implementation is very general and straightforward. What do you think ?

What is the circular dependency? The contextRegister is basically copying the fields from TideContextPolicy and defining new methods so I'd really prefer to condense these into one struct and just configure the packages and imports properly to avoid a circular dependency.

The tide status shouldn't need to be hard coded? It can be passed down from the tide package.

The circular deps is on the Context object defined in prow/tide/tide.go

cjwagner · 2018-05-15T18:45:13Z

prow/tide/tide.go

@@ -991,6 +1002,10 @@ func (c *Controller) syncSubpool(sp subpool) (Pool, error) {
 	if err != nil {
 		return Pool{}, fmt.Errorf("error determining required presubmits: %v", err)
 	}
+	cr, err := newContextRegisterFromConfig(sp.org, sp.repo, sp.branch, c.ca.Config())
+	if err != nil {
+		return Pool{}, fmt.Errorf("error parsing branch protection: %v", err)


This error message is no longer accurate.

sebastienvas · 2018-05-16T06:57:33Z

@cjwagner Please take a look

cjwagner

Just a couple nits. This is looking ready.

cjwagner · 2018-05-17T02:05:08Z

prow/config/tide.go

+
+// RegisterRequiredContexts register required contexts.
+// Once required contexts are registered other contexts will be considered optional.
+func (cp *TideContextPolicy) RegisterRequiredContexts(c ...string) {


This func is no longer needed.

cjwagner · 2018-05-17T02:10:30Z

prow/config/tide.go

+}
+
+// RegisterOptionalContexts registers optional contexts
+func (cp *TideContextPolicy) RegisterOptionalContexts(c ...string) {


It might make more sense to just pass the tide context name to GetTideContextPolicy and have that func make the tide context required. That is all that RegisterOptionalContexts is used for now.

Not a big deal either way.

We can also just revert part of my change such that tide skip its own status. What do you think ?

sebastienvas · 2018-05-17T17:15:43Z

@cjwagner Please take a look. I ll be deploying this today for istio. Will keep you up to date.

cjwagner

/lgtm

k8s-ci-robot · 2018-05-17T20:51:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cjwagner, sebastienvas

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~~ [cjwagner]

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

sebastienvas requested review from cjwagner and stevekuznetsov as code owners May 9, 2018 20:56

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/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 9, 2018

sebastienvas mentioned this pull request May 9, 2018

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

Closed

k8s-ci-robot added area/prow Issues or PRs related to prow kind/feature Categorizes issue or PR as related to a new feature. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 9, 2018

stevekuznetsov reviewed May 9, 2018

View reviewed changes

0xmichalis mentioned this pull request May 11, 2018

allow retester to work with optional contexts #8008

Closed

sebastienvas force-pushed the tide branch from f76d718 to 0653446 Compare May 12, 2018 06:49

sebastienvas force-pushed the tide branch from 0653446 to 7b493b5 Compare May 12, 2018 07:08

sebastienvas commented May 12, 2018

View reviewed changes

cjwagner approved these changes May 15, 2018

View reviewed changes

sebastienvas force-pushed the tide branch from 7b493b5 to 48fbe52 Compare May 16, 2018 06:56

k8s-ci-robot added 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 May 16, 2018

sebastienvas force-pushed the tide branch from 48fbe52 to 3c41771 Compare May 16, 2018 15:27

sebastienvas assigned cjwagner May 16, 2018

cjwagner reviewed May 17, 2018

View reviewed changes

sebastienvas added 4 commits May 17, 2018 10:07

adds a context options in tide config

12e10bd

Fix comments

7a5a28b

Implement contextChecker interface on TideContextPolicy

c69a381

Remove register functions

cc361a6

sebastienvas force-pushed the tide branch from 3c41771 to cc361a6 Compare May 17, 2018 17:07

Update readme

a9509f5

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 17, 2018

cjwagner approved these changes May 17, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 17, 2018

k8s-ci-robot merged commit d387f27 into kubernetes:master May 17, 2018

sebastienvas deleted the tide branch May 17, 2018 23:54

Adds ability to set required and optional checks #7983

Adds ability to set required and optional checks #7983

Conversation

sebastienvas commented May 9, 2018 • edited Loading

stevekuznetsov commented May 9, 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 May 10, 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

sebastienvas May 10, 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

sebastienvas commented May 10, 2018

stevekuznetsov commented May 11, 2018

sebastienvas commented May 12, 2018

sebastienvas 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastienvas commented May 16, 2018

cjwagner 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

sebastienvas commented May 17, 2018

cjwagner left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 17, 2018

sebastienvas commented May 9, 2018 •

edited

Loading

sebastienvas May 10, 2018 •

edited

Loading

sebastienvas May 10, 2018 •

edited

Loading