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

xiangpengzhao commented 7 years ago

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 commented 7 years ago

cc @kubernetes/sig-contributor-experience-misc

xiangpengzhao commented 7 years ago

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 commented 7 years ago

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

grodrigues3 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

/remove-lifecycle stale

krmayankk commented 6 years ago

@xiangpengzhao are you still working on this ?

idealhack commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

/area contributor-guide

nikhita commented 6 years ago

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 commented 6 years ago

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

fejta-bot commented 6 years ago

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 commented 6 years ago

/remove-lifecycle stale

On Tue, Oct 16, 2018 at 12:26 PM fejta-bot notifications@github.com 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 commented 6 years ago

/remove-lifecycle stale

fejta-bot commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

/remove-lifecycle rotten

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

/assign

nikhita commented 5 years ago

/unassign /help

nikhita commented 5 years ago

/kind documentation /priority important-longterm

ezzoueidi commented 5 years ago

/assign /lifecycle active

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

nikhita commented 5 years ago

/remove-help since @nzoueidi is working on this

fejta-bot commented 5 years ago

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

markjacksonfishing commented 5 years ago

/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 commented 5 years ago

@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 commented 4 years ago

/lifecycle frozen

vishakhanihore commented 4 years ago

/assign

kubernetes / community

Document in what cases to "/ok-to-test" #923