Test-infra: Support command "/lgtm trivial" or something to make PR not need approval

Created on 20 Jul 2017 · 7Comments · Source: kubernetes/test-infra

Currently a PR needs an associated issue. If no issue, it needs /approve no-issue to get approved. When it has both lgtm and approved labels, it's then allowed to be merged. lgtm-ers and approvers may be different people, so the PR should wait for lgtm from one people and approved from other one(s). However, it's definitely unnecessary for those trivial PRs, especially the PRs which fix typos, broken links, etc. See example: https://github.com/kubernetes/kubernetes/pull/49294, it has a lgtm and is waiting for approval.

So I suggest a command /lgtm trivial for those PRs. When an assignee comments this in a PR, it will get both lgtm and approved labels.

@crimsonfaith91 @grodrigues3 @spiffxp @spxtr WDYT?

Source

xiangpengzhao

Most helpful comment

I am against this. We should not allow bypassing approvals as much trivial a PR can be.

kargakis on 20 Jul 2017

👍4

All 7 comments

@xiangpengzhao Good idea! Let's see opinions from others. :)

crimsonfaith91 on 20 Jul 2017

I am against this. We should not allow bypassing approvals as much trivial a PR can be.

kargakis on 20 Jul 2017

👍4

@kargakis Agreed too. A trivial PR can be dangerous. :)

crimsonfaith91 on 20 Jul 2017

The lgtm-ers should be very sure that a PR is indeed trivial when they are ready to /lgtm trivial. OTOH, approvers might miss potential bugs even though we request approval from them.

xiangpengzhao on 20 Jul 2017

This is a balance between convenience and safety. I agree with that safety should be our first choice. The more steps (lgtm + approved), the safer it seems to be:)

xiangpengzhao on 20 Jul 2017

Yeah, I don't want to provide a mechanism to circumvent approvers (which is our primary source of gating since lgtm can be written by anyone in the org now). Can we close? Or are there other suggestions?