Repository: jenkins-x/lighthouse (go)

Issue:
Run/require certain contexts based on files changed in PR
Right now, we've got Prow's behavior of having contexts either be required or not required, with no granularity. A really, really nice feature would be to say "the `frontend` context is only required and should only be run if files in the `ui` directory have changed in this PR", or "if the only file(s) changed in this PR are in the `examples` subdirectory, don't require/run any contexts".

Pull request:
fix: Support requiring for contexts with conditional execution
Using the `runIfChanged` JX scheduler field for pre- and post-submits (`run_if_changed` in the Prow/Lighthouse configuration for presubmits that we actually generate from the jx scheduler configuration) and the `alwaysRun` jx scheduler field, you can specify a regex in `runIfChanged` and `false` in `alwaysRun`, in which case the context will only run when the list of files changed in the PR matches the regex.

That's been in place for a while, but until this change, the jx scheduler->config generation would get things confused and make any context specified in `policy.requiredStatusContexts` be treated as required, whether it's automatically run/conditionally run/etc. This is largely because we kinda did things a bit dumb in how we generate the branch protection required status checks. Prow has a cronjob that will update GitHub branch protection rules based on the "branch protection" configuration in Prow, but we've never used that with jx, and we don't support it in Lighthouse, at least not at this point, but since I don't want to rule out the possibility in the future, I'd like to leave this janky setup as is for now.

So now we will remove anything from the "required" list if it's also in the "required if present" list, just to be safe.

fixes #435

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

Patch:
diff --git a/pkg/config/keeper.go b/pkg/config/keeper.go
index 5de83877..88198e96 100644
--- a/pkg/config/keeper.go
+++ b/pkg/config/keeper.go
@@ -518,6 +518,10 @@ func (c Config) GetKeeperContextPolicy(org, repo, branch string) (*KeeperContext
 		}
 	}
 
+	// Remove anything from the required list that's also in the required if present list, since that may have been
+	// duplicated by branch protection.
+	required.Delete(requiredIfPresent.List()...)
+
 	t := &KeeperContextPolicy{
 		RequiredContexts:          required.List(),
 		RequiredIfPresentContexts: requiredIfPresent.List(),


Test patch:
diff --git a/pkg/config/keeper_test.go b/pkg/config/keeper_test.go
new file mode 100644
index 00000000..a1670d4a
--- /dev/null
+++ b/pkg/config/keeper_test.go
@@ -0,0 +1,319 @@
+package config
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestConfigGetKeeperContextPolicy(t *testing.T) {
+	cases := []struct {
+		name                 string
+		bpOrgs               map[string]Org
+		presubmits           []Presubmit
+		skipUnknownContexts  bool
+		fromBranchProtection bool
+
+		expectedRequired  []string
+		expectedOptional  []string
+		expectedIfPresent []string
+	}{
+		{
+			name: "basic",
+			presubmits: []Presubmit{
+				{
+					AlwaysRun: true,
+					Reporter: Reporter{
+						Context:    "always-run",
+						SkipReport: false,
+					},
+				},
+				{
+					RegexpChangeMatcher: RegexpChangeMatcher{
+						RunIfChanged: "foo",
+					},
+					AlwaysRun: false,
+					Reporter: Reporter{
+						Context:    "run-if-changed",
+						SkipReport: false,
+					},
+				},
+				{
+					AlwaysRun: false,
+					Reporter: Reporter{
+						Context:    "not-always",
+						SkipReport: false,
+					},
+				},
+				{
+					AlwaysRun: true,
+					Reporter: Reporter{
+						Context:    "skip-report",
+						SkipReport: true,
+					},
+					Brancher: Brancher{
+						SkipBranches: []string{"master"},
+					},
+				},
+				{
+					AlwaysRun: true,
+					Reporter: Reporter{
+						Context:    "optional",
+						SkipReport: false,
+					},
+					Optional: true,
+				},
+			},
+			expectedRequired:  []string{"always-run"},
+			expectedIfPresent: []string{"run-if-changed", "not-always"},
+			expectedOptional:  []string{"optional"},
+		},
+		{
+			name: "from branch protection",
+			presubmits: []Presubmit{
+				{
+					AlwaysRun: true,
+					Reporter: Reporter{
+						Context:    "always-run",
+						SkipReport: false,
+					},
+				},
+				{
+					RegexpChangeMatcher: RegexpChangeMatcher{
+						RunIfChanged: "foo",
+					},
+					AlwaysRun: false,
+					Reporter: Reporter{
+						Context:    "run-if-changed",
+						SkipReport: false,
+					},
+				},
+				{
+					AlwaysRun: false,
+					Reporter: Reporter{
+						Context:    "not-always",
+						SkipReport: false,
+					},
+				},
+				{
+					AlwaysRun: true,
+					Reporter: Reporter{
+						Context:    "skip-report",
+						SkipReport: true,
+					},
+					Brancher: Brancher{
+						SkipBranches: []string{"master"},
+					},
+				},
+				{
+					AlwaysRun: true,
+					Reporter: Reporter{
+						Context:    "optional",
+						SkipReport: false,
+					},
+					Optional: true,
+				},
+			},
+			fromBranchProtection: true,
+			bpOrgs: map[string]Org{
+				"o": {
+					Policy: Policy{},
+					Repos: map[string]Repo{
+						"r": {
+							Policy: Policy{
+								RequiredStatusChecks: &ContextPolicy{
+									Contexts: []string{
+										"always-run",
+										"run-if-changed",
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+			expectedRequired:  []string{"always-run"},
+			expectedIfPresent: []string{"run-if-changed", "not-always"},
+			expectedOptional:  []string{"optional"},
+		},
+		{
+			name: "from branch protection with unknown context",
+			presubmits: []Presubmit{
+				{
+					AlwaysRun: true,
+					Reporter: Reporter{
+						Context:    "always-run",
+						SkipReport: false,
+					},
+				},
+				{
+					RegexpChangeMatcher: RegexpChangeMatcher{
+						RunIfChanged: "foo",
+					},
+					AlwaysRun: false,
+					Reporter: Reporter{
+						Context:    "run-if-changed",
+						SkipReport: false,
+					},
+				},
+				{
+					AlwaysRun: false,
+					Reporter: Reporter{
+						Context:    "not-always",
+						SkipReport: false,
+					},
+				},
+				{
+					AlwaysRun: true,
+					Reporter: Reporter{
+						Context:    "skip-report",
+						SkipReport: true,
+					},
+					Brancher: Brancher{
+						SkipBranches: []string{"master"},
+					},
+				},
+				{
+					AlwaysRun: true,
+					Reporter: Reporter{
+						Context:    "optional",
+						SkipReport: false,
+					},
+					Optional: true,
+				},
+			},
+			fromBranchProtection: true,
+			bpOrgs: map[string]Org{
+				"o": {
+					Policy: Policy{},
+					Repos: map[string]Repo{
+						"r": {
+							Policy: Policy{
+								RequiredStatusChecks: &ContextPolicy{
+									Contexts: []string{
+										"always-run",
+										"run-if-changed",
+										"non-lighthouse-job",
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+			expectedRequired:  []string{"always-run", "non-lighthouse-job"},
+			expectedIfPresent: []string{"run-if-changed", "not-always"},
+			expectedOptional:  []string{"optional"},
+		},
+		{
+			name: "from branch protection skipping unknown context",
+			presubmits: []Presubmit{
+				{
+					AlwaysRun: true,
+					Reporter: Reporter{
+						Context:    "always-run",
+						SkipReport: false,
+					},
+				},
+				{
+					RegexpChangeMatcher: RegexpChangeMatcher{
+						RunIfChanged: "foo",
+					},
+					AlwaysRun: false,
+					Reporter: Reporter{
+						Context:    "run-if-changed",
+						SkipReport: false,
+					},
+				},
+				{
+					AlwaysRun: false,
+					Reporter: Reporter{
+						Context:    "not-always",
+						SkipReport: false,
+					},
+				},
+				{
+					AlwaysRun: true,
+					Reporter: Reporter{
+						Context:    "skip-report",
+						SkipReport: true,
+					},
+					Brancher: Brancher{
+						SkipBranches: []string{"master"},
+					},
+				},
+				{
+					AlwaysRun: true,
+					Reporter: Reporter{
+						Context:    "optional",
+						SkipReport: false,
+					},
+					Optional: true,
+				},
+			},
+			fromBranchProtection: true,
+			bpOrgs: map[string]Org{
+				"o": {
+					Policy: Policy{},
+					Repos: map[string]Repo{
+						"r": {
+							Policy: Policy{
+								RequiredStatusChecks: &ContextPolicy{
+									Contexts: []string{
+										"always-run",
+										"run-if-changed",
+										"non-lighthouse-job",
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+			skipUnknownContexts: true,
+			expectedRequired:    []string{"always-run", "non-lighthouse-job"},
+			expectedIfPresent:   []string{"run-if-changed", "not-always"},
+			expectedOptional:    []string{"optional"},
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+
+			if err := SetPresubmitRegexes(tc.presubmits); err != nil {
+				t.Fatalf("could not set regexes: %v", err)
+			}
+			presubmits := map[string][]Presubmit{
+				"o/r": tc.presubmits,
+			}
+			cfg := Config{
+				JobConfig: JobConfig{
+					Presubmits: presubmits,
+				},
+				ProwConfig: ProwConfig{
+					Keeper: Keeper{
+						ContextOptions: KeeperContextPolicyOptions{
+							KeeperContextPolicy: KeeperContextPolicy{
+								SkipUnknownContexts:  &tc.skipUnknownContexts,
+								FromBranchProtection: &tc.fromBranchProtection,
+							},
+						},
+					},
+				},
+			}
+			if tc.bpOrgs != nil {
+				cfg.ProwConfig.BranchProtection = BranchProtection{
+					ProtectTested: true,
+					Orgs:          tc.bpOrgs,
+				}
+			}
+			ctxPolicy, err := cfg.GetKeeperContextPolicy("o", "r", "master")
+			assert.NoError(t, err)
+			assert.NotNil(t, ctxPolicy)
+
+			assert.ElementsMatch(t, tc.expectedRequired, ctxPolicy.RequiredContexts)
+			assert.ElementsMatch(t, tc.expectedIfPresent, ctxPolicy.RequiredIfPresentContexts)
+			assert.ElementsMatch(t, tc.expectedOptional, ctxPolicy.OptionalContexts)
+		})
+	}
+}