Test-infra: Prow should make tests that are asked for via a comment sticky to the PR

[luxas]

Issue noticed in https://github.com/kubernetes/kubernetes/pull/36050

We commented "@k8s-bot build this" in the beginning of the thread, but then a new (bad) commit was uploaded and the cross-build test wasn't run for the second commit.

I and @ixdy took for granted that it ran, and since everything was green, we lgtm'd, which made the cross-build CI job fail in a very untimely manner, when kubeadm was gonna be released.

So to fix this issue, any tests that are invoked once should be attached to that PR and be run normally like it would be a pre-submit test.

Also, I'd like to make the cross-build test a pre-submit test (and have heard others saying the same thing)
It wouldn't have to be blocking, but at least people will notice if it's failing because of their PR

cc @kubernetes/sig-testing @mml

[spxtr]

@luxas to make it run on all presubmits, set always_run to true in https://github.com/kubernetes/test-infra/blob/master/prow/presubmit.yaml, and also make its trigger regex work for @k8s-bot test this.

[luxas]

Can we make it per-PR now?
I see the cross-build as absolutely critical now when we have Windows Containers in the v1.5 release branch, it nearly broke the release today: https://github.com/kubernetes/kubernetes/pull/39731

Without it, one could break windows (or arm or osx or anything else) on the release branch without noticing until we actually do a release. The same goes for the master branch, the cross-build should be healthy at all times. At least people would notice if their PRs are breaking the build before we merge them.

cc @pires @feiskyer @resouer @brendandburns @colemickens @ixdy @vishh

[resouer]

I think it's reasonable to have a cross-build CI check item. Windows & Mac users are all important factors of the community.

[rmmh]

Yes, they're important, but is running another long job to cross build every pr worth slowing down everything by 10%? Almost all that utility is still given with a conditional check on the small number of PRs likely to break things

[resouer]

What about enable it for specific files and directories modification? For example, container runtime specific PRs which change code in pkg/kubelet/{dockershim, dockertools}?

[pires]

Since Windows Containers are supported since 1.5, at least, for now, we need to make sure kube-proxy and kubelet build on Windows.

[vishh]

Do we have data on how long cross builds would take? We could optimize
those builds if required to have them run as part of pre-submit.

On Wed, Jan 11, 2017 at 6:53 PM, Paulo Pires [e-mail]
wrote:

Since Windows Containers are supported since 1.5, at least, for now, we
need to make sure kube-proxy and kubelet build on Windows.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/test-infra/issues/1351#issuecomment-272062166,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGvIKE5nZr5hK5cvwPvympKYZyFATq_7ks5rRZWxgaJpZM4LK_EM
.

[ixdy]

another related case: if you explicitly request testing for a job that isn't set to always run (e.g. with /test some-job-name), and that job fails, /retest won't retrigger that test, even though the comment left by the bot suggests it will.

[fejta]

/unassign @spxtr

Anyone want to work on this? Involves improving https://github.com/kubernetes/test-infra/tree/master/prow/plugins/trigger

[kargakis]

We definitely want this. @stevekuznetsov maybe something Daniel can take a stab at?

[stevekuznetsov]

Unclear. Slated to begin helping with Caesar's work next sprint. I can try to take a look too if ten million things don't catch fire next week.

[kargakis]

you asked for it

/assign @stevekuznetsov

[stevekuznetsov]

lol

[cpanato]

there is anyone working on this? I would like to try this one.

[stevekuznetsov]

/assign @cpanato
All yours!

[k8s-ci-robot]

@stevekuznetsov: GitHub didn't allow me to assign the following users: cpanato.

Note that only kubernetes members can be assigned.

In response to this:

/assign @cpanato
All yours@

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[stevekuznetsov]

Hrm -- all yours in spirit even if the bots don't want to behave. :)

[BenTheElder]

GitHub won't let the bot behave :(
You have to be an org member

On Tue, Dec 19, 2017, 11:07 Steve Kuznetsov [e-mail]
wrote:

Hrm -- all yours in spirit even if the bots don't want to behave. :)

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/test-infra/issues/1351#issuecomment-352856441,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA4Bq04QHU1RMRXXW16NnSxOQ3fyxnbpks5tCAmGgaJpZM4LK_EM
.

[cjwagner]

Is this suggesting that if I /test foo for an optional job foo and later decide that foo doesn't need to be passing on my PR after I change things, I have to close the PR and create a brand new one?

[kargakis]

@cjwagner you can use /skip and rerun any other optional jobs you may wish to include in the final results. Or we could make /skip accept an argument to override individual optional contexts.

[cjwagner]

Ok, if the logic that looks for /test foo commands to make foo sticky disregards any /test foo commands that occur before a /skip then this makes sense to me. :+1:

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[cpanato]

/remove-lifecycle stale

[cpanato]

@luxas @fejta hello, I need some help to explain to me what needs to be done here.

[luxas]

@spxtr @cjwagner @fejta @kargakis Please tell @cpanato what exactly to do here wrt the implementation

[cjwagner]

Is this really worth the complexity and API token cost? We currently can determine what tests need to be run based on just the org, repo, and conditionally (if there is a run_if_changed job for the repo) the files that are modified by the PR. Adding an unconditional dependency on the PR's comments seems like a coupling we should try to avoid.
If we need a way to make jobs 'stick' to a PR I'd prefer storing that state in something other than Github comments.

[luxas]

I don't care strongly about this. We might as well not do it, and mark it as the expected behavior. That's totally fine by me, but I just found it a bit unintuitive at the time. Please doc this instead.

[cpanato]

I will search another ticket to try to work on.