Community: Document in what cases to "/ok-to-test"

[xiangpengzhao]

As a new member, I want to help trigger tests for those PRs which have needs-ok-to-test. But I'm not sure in what cases I can comment /ok-to-test to trigger tests.

As what I can think about, PRs with the below cases shouldn't be given a /ok-to-test.

• with label cncf-cla: no
• with label do-not-merge
• with label needs-rebase
• created by mistake (without meaningful code patch. e.g. https://github.com/kubernetes/kubernetes/pull/50074)

cc @cblecker @grodrigues3

[xiangpengzhao]

cc @kubernetes/sig-contributor-experience-misc

[xiangpengzhao]

We may want to document it here: https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md#the-testing-and-merge-workflow

[MHBauer]

Not a member. Yes, I agree would be good to start with a list of when not to test.

[grodrigues3]

This was originally created to prevent people from mining bitcoins and running other malicious code using our ci/cd.

So, yeah, the cases you mentioned as well as the above.

Also, @xiangpengzhao if you decide to take this, can you update the doc to include the new command /ok-to-test rather than @k8s-bot ok to test

[cblecker]

For me, it also depends on the size of the PR. If it's XS/S/M, then I'm more likely to ok-to-test a PR that I'm not assigned.. if it's a L/XL/XXL, then I'll usually leave it for the assigned reviewers as there is more to go over, and there may need to be a larger overhaul of the PR before it's even worth testing.

[xiangpengzhao]

Also, @xiangpengzhao if you decide to take this, can you update the doc to include the new command /ok-to-test rather than @k8s-bot ok to test

@grodrigues3 There are also other places which should be updated. I had an issue tracking such cases: https://github.com/kubernetes/community/issues/834 But I'm not ready for this yet :) I'm afraid I'm not so sure about some of the exact details of the current process.

[xiangpengzhao]

For me, it also depends on the size of the PR. If it's XS/S/M, then I'm more likely to ok-to-test a PR that I'm not assigned.. if it's a L/XL/XXL, then I'll usually leave it for the assigned reviewers as there is more to go over, and there may need to be a larger overhaul of the PR before it's even worth testing.

:+1:

Another concern, is it ok if I lgtm some PRs which aren't assigned to me? These PRs are trivial, e.g. typos, log message changes. Just want to lgtm them to less other reviewers' burden. I believe they're busier than I am:)

[tpepper]

I proposed some of this "trivial PR" scrubbing as a chop wood, carry water entry point for new reviewers going through the new https://git.k8s.io/community/mentoring/group-mentoring.md mentor program and it's even listed in https://github.com/kubernetes/community/blob/master/mentoring/group-mentee-guide.md

Perhaps @parispittman can at the end of the first cohort's test period get the participants to distill into a document how they found was best to coalesce and deal with this class of PRs.

[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

[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

[krmayankk]

/remove-lifecycle stale

[krmayankk]

@xiangpengzhao are you still working on this ?

[idealhack]

Do we need to continue the work on this?

I've been triggering tests in some PRs in oktotest.com recently. For now, the query is is:open is:pr org:kubernetes -repo:kubernetes/charts label:needs-ok-to-test -label:needs-rebase

UPDATE: more shortcuts https://twitter.com/jaydumars/status/972956233559834633

[nikhita]

Do we need to continue the work on this?

I'm not sure if this is still required but I have a question:

Should I ok-to-test PRs:

• targeting areas that I am not involved in
• haven't had any reviews yet from folks who should be reviewing the PR
• but don't look harmful (in terms of bitcoin mining, etc).

I don't ok-to-test such PRs now, but I see some merit in ok-to-test-ing them. For example, the author can come to know places where their code fails, fix them before a reviewer gets to it, etc.

The concern I have about ok-to-test-ing such PRs:

• is it a waste of our resources if we start testing on such PRs because the PR might need changes after reviewing?
• it would be a waste of our resources if the reviewer decides that the PR is not needed and should be closed.

I've been triggering tests in some PRs in oktotest.com recently. For now, the query is is:open is:pr org:kubernetes -repo:kubernetes/charts label:needs-ok-to-test -label:needs-rebase

@idealhack thanks for doing that! I saw that and also noticed that it's smoothened out our contributor experience a lot because they don't have to wait for a long time until a maintainer/contributor comments on their PR! :heart:

[cblecker]

Do we need to continue the work on this?

I think having clear guidance on this is still worthwhile.

If there is an obvious problem and we know tests are going to fail, then I usually call that out and won't okay it for test until that's fixed.

I also try to determine how complex a change is, and it's likelihood of being accepted before kicking off CI. Here's an example:

• First time contributor creates a size/S PR to fix spelling mistake in an error message, or provide more details in a debug output. Small change, no major functionality changes, unlikely to be controversial. Will kick off CI.
• First time contributor creates a size/XXL PR to add a new API field to the scheduler. No feature ticket or kep is logged, and it doesn't appear to have been passed by the sig for review. API changes require a lengthy review process, and a first time contributor is unlikely to know our conventions/process if they haven't talked to the sig yet. Will not okay to test the PR, as it's unlikely to be accepted in it's current form, and it would be disingenuous to have the author spend more time to fix tests with the PR in this state.

It's things like the above that I'd like to codify, as there are situations where not okaying to test actually provides the better experience to new contributors.

[nikhita]

there are situations where not okaying to test actually provides the better experience to new contributors.

oh yeah, good point! :+1:

I can try to take a look at documenting this over the weekend. We can, perhaps, discuss more things to be added/removed in the PR.

[nikhita]

/area contributor-guide

[nikhita]

Another concern, is it ok if I lgtm some PRs which aren't assigned to me? These PRs are trivial, e.g. typos, log message changes.

Missed this question a few comments up.

Good question, @xiangpengzhao! For me, personally, I lgtm only when I am _super, super, super_ sure about the PR. I think this holds true to all types of PRs - big changes or small ones like typos, etc.

I think it is ok to lgtm typos, etc even if you are not a reviewer/approver for that area of the code - if the fixes make sense and you are sure about it. If you are not an approver for that area of the code, it needs to go through an approval anyway -- which should catch if things need changing.

Curious to hear what others think about this though. :)

[xiangpengzhao]

@nikhita thanks for picking this! I totally agree with you :)

[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

[nikhita]

/remove-lifecycle stale

On Tue, Oct 16, 2018 at 12:26 PM fejta-bot [e-mail] wrote:

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
https://github.com/fejta.
/lifecycle stale

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

[idealhack]

/remove-lifecycle stale

[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

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

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 rotten

[nikhita]

/remove-lifecycle rotten

I think it would still be good to have this explicitly documented in the issue triage doc.

/assign

[nikhita]

/unassign
/help

[nikhita]

/kind documentation
/priority important-longterm

[nzoueidi]

/assign
/lifecycle active

I am working on this, expect a PR in the few upcoming days.

[nikhita]

/remove-help
since @nzoueidi is working on this

[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

[markyjackson-taulia]

/remove-lifecycle stale
@nikhita would this make sense to add to the ongoing developer guide work I am doing? If so, I can assign this to me but I wanted to check with you 1st

[nikhita]

@nikhita would this make sense to add to the ongoing developer guide work I am doing? If so, I can assign this to me but I wanted to check with you 1st

Yes :))

/assign @markyjackson-taulia

[vishakhanihore]

/lifecycle frozen

[vishakhanihore]

/assign