Kubebuilder: Prow tests should be required for merge PR

[mengqiy]

It seems we don't require prow tests to be passing before tide merging a PR.
Currently, travis seems to be the only required check.

[mengqiy]

@alexeldeib Do you have bandwidth to take a look at this?

[alexeldeib]

Yep! I think we just need to flip all of the optional: true fields to false, like here: https://github.com/kubernetes/test-infra/blob/ac16891f13cdcb390b9d603f6318831d6c5e7031/config/jobs/kubernetes-sigs/kubebuilder/kubebuilder-presubmits.yaml#L6

While I'm at it, do you want to add tests against 1.17 or any of the newer patch versions?

[mengqiy]

do you want to add tests against 1.17

Yes, that will be helpful. Please make this one optional, since we haven't test it against 1.17. It something isn't working, we don't want it to be very disruptive for kubebuilder contributors.

any of the newer patch versions?

If they are trivial to update, then yes please. Otherwise, just keep it as is.

[alexeldeib]

/assign

[Adirio]

In PRs I'm only seeing cla/linuxfoundation marked as required but I guess you are talking about different requirements. I'm guessing the ones that GitHub marks as required are those required for manual merging, and that you are talking about the ones that @k8s-ci-robot requires before merging. Right? Where can I find this info?

Also, another related question, should we remove these tests from Travis? I think they are overlapping.

[alexeldeib]

Here's the blurb for required status in prow, not particularly comprehensive, but basically yes it's what you said: https://github.com/kubernetes/test-infra/blob/dd3ff6d1aa7e27d20546ea954e30e8295389c44f/prow/jobs.md#requiring-job-statuses

I see there's already a good discussion going on re: overlapping tests. I'm not 100% sure what the best answer is; I don't love that we have tests split because of OS dependencies right now. I'll catch up on the other thread

[Adirio]

@alexeldeib @mengqiy I think #1353 is the first case with the changes done in kubernetes/test-infra#16112. As you can see, pull-kubebuilder-e2e-k8s-1-14-1, pull-kubebuilder-e2e-k8s-1-15-3, pull-kubebuilder-e2e-k8s-1-16-2 and pull-kubebuilder-test were marked as required by github. Does that mean that a mantainer is unable to merge the PR manually?

Also, I noticed that there is no pull-kubebuilder-e2e-k8s-1-17-0 yet, it may be launched later as it is optional.

EDIT: the required prow tests haven't report any status yet, nor have a "Details" link to check whats going on.

[alexeldeib]

The author in #1353 wasn't a kubernetes-sigs org member, so it required a manual "ok-to-test" for the tests to run. I can see now the 1.17 job was also triggered once the tests did start actually executing. If I had to guess, when the PR is blocked on merge, only the required tests are shown, and the optional one only gets triggered on the actual run -- feels like a github quirk more than anything.

[alexeldeib]

Does that mean that a mantainer is unable to merge the PR manually?

No, this is even slightly backwards -- the benefit is that prow can automatically manage the merge queue for us once all required approvals have been met, but a maintainer can manually override this. The alternative is manually merging every PR.