Newly added ProwJobs don't honor skip_if_changed, etc.. configurations

[smg247]

When a new check is added to prow, it is added to (and triggered on) all currently opened PRs in the repo regardless of the skip_if_changed, and only_run_if_changed configs. This causes massive disruption in some cases where there are many opened PRs in the repo. When a new PR is opened after the ProwJob has been added, the configurations are honored.

[smg247]

/kind bug

[petr-muller]

Long time ago this had an issue in k/test-infra, but it probably was not migrated (or maybe rot before the migration even started). This is caused by the difference in how the jobs are triggered when they run "normally" on a GH event, and when they are triggered by the status-reconciler after job configuration changed. I think status-reconciler wants to save GH tokens or something and only inspects the open PRs to trigger newly added jobs for, but does not check out the code and test the conditional trigger clauses against the PR content. EDIT: Incorrect

Not sure what is the best way to address this - either improve status-reconciler to fully evaluate each PR for trigger feasibility, or come up with some alternative.

[hown3d]

We stumbled upon this issue today as well. As I can see, in the status reconciler, the custom filter defined here:

prow/pkg/statusreconciler/controller.go

Lines 244 to 249 in d572f85

	// we want to appropriately trigger and skip from the set of identified presubmits that were
	// added. we know all of the presubmits we are filtering need to be forced to run, so we can
	// enforce that with a custom filter
	filter := pjutil.NewArbitraryFilter(func(p config.Presubmit) (shouldRun bool, forcedToRun bool, defaultBehavior bool) {
	return true, false, true
	}, "inline-filter")

sets the default behavior for this filter to true.

In the pjutil.FilterPresubmits function, the defaultBehavior returned by the filter is passed to the ShouldRun() method of presubmits.
The function returns true if either there are changes match the regexes or the defaultBehavior is true.

prow/pkg/config/jobs.go

Lines 479 to 494 in d572f85

	// ShouldRun determines if the presubmit should run against a specific
	// base ref, or in response to a set of changes. The latter mechanism
	// is evaluated lazily, if necessary.
	func (ps Presubmit) ShouldRun(baseRef string, changes ChangedFilesProvider, forced, defaults bool) (bool, error) {
	if !ps.CouldRun(baseRef) {
	return false, nil
	}
	if ps.AlwaysRun {
	return true, nil
	}
	if forced {
	return true, nil
	}
	determined, shouldRun, err := ps.RegexpChangeMatcher.ShouldRun(changes)
	return (determined && shouldRun) || defaults, err
	}

Since the defaultBehavior always evaluates to true, the presubmits are always triggered.

Therefore I suggest to set the defaultBehavior to false. WDYM?