Bot configuration change for k/community repo

[Christoph Blecker]

Hey all,

Looking to make two changes to the bot in k/community:

• Disable implicit self-approve by the author (the author would have to manually /approve if they intend to approve it)
  
• Changes the merge method to squash.
  

The first point is in line with the idea of least astonishment, and make /approve be an explicit action in k/community.. this piggy backs on the discussion here: https://groups.google.com/forum/#!topic/kubernetes-dev/PJJxV9roXI8

The second creates a more concise git history for a repo that is mostly documentation. The commit will also more clearly link back to the PR that created it. This is what kubernetes/website is doing, for an example: https://github.com/kubernetes/website/commits/master

Looking for lazy consensus here. If you object (or strongly support) please comment on the PR:

https://github.com/kubernetes/test-infra/pull/7336

Cheers,

Christoph

[Timothy Pepper]

I feel pretty strongly that it is important for individuals to form PRs in a way that eases review first and foremost.  This biases towards smaller commits, with more verbose and targeted commit messages.  When this happens well, the git history is concise and logical

 

On the other hand you’re not saying logical history is an aim.  As you note there’s not a need in this repo for bisect-ability so maybe logical history is an implicit non-goal?  If that’s the case, I guess I can be ok with this if I have to.  For just this repo.  :mildlyfrownyface:

 

There’s an argument to be made that this isn’t the place to make contributors work really hard at patch rebasing, refactoring, squashing, skills that could turn away casual contributors if expected of them.  For some of those contributors though we’ll be starting them into a bad habit somebody else will have to correct later.

 

 

-- 

Tim Pepper

Open Source Cloud Technologies Lead

VMware Open Source Technology Center

--
You received this message because you are subscribed to the Google Groups "kubernetes-wg-contribex" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-wg-con...@googlegroups.com.
To post to this group, send email to kubernetes-...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-wg-contribex/c18eadf4-c853-47f9-b91a-69d864e3ed55%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Garrett Rodrigues]

Both sound fine to me.

This does impact consistency though if we're only making these changes for the community repo.

Is this a test prior to making changes across the broader K8s ecosystem or is there some reason we don't think these changes will work in other repos?

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-wg-contribex/60F0606C-69E3-4B06-BB9B-D1A00A7EEFB3%40vmware.com.

[Christoph Blecker]

On the approval change: This is kind of a canary change for *possible* roll out in other places. The author may want a specific type of review or approval for someone else, and may not want to implicitly approve anything they create. For example, if I create a kep for something, as a top-level approver in k/community, it would be default approved.. which likely would not be my intention for a kep. I'd like to see how this flow works in k/community, and if it would be something worth rolling out other places.

On the squash change: I think squashing all PRs by default is something that only works in some repos. For something like k/community, having PR authors make small commits and iterations makes review much easier, but can leave a messy git history in master. For documentation of this kind, doing bi-sects and blames outside of the PR as a whole I don't think will be a problem.

It solves for cases like https://github.com/kubernetes/community/pull/1938.. Small PR, valuable fix, new contributor. He created a second commit for feedback. Is that second commit needed? No. Is it worth turning him away to squashing locally? Nope. 

A couple examples of use cases that would be solved:

- https://github.com/kubernetes/community/pull/1938 - Small PR, valuable fix, new contributor. He created a second commit for feedback. Is that second commit needed? No. Is it worth turning him away to squashing locally? Nope. 

- https://github.com/kubernetes/community/pull/1654 - Paris used the Github UI to make this change. Three commits, as there was feedback.. but to squash into one, she'd need to clone the branch locally and squash.

To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-wg-contribex+unsub...@googlegroups.com.
To post to this group, send email to kubernetes-wg-contribex@googlegroups.com.

To view this discussion on the web visit

https://groups.google.com/d/msgid/kubernetes-wg-contribex/c18eadf4-c853-47f9-b91a-69d864e3ed55%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "kubernetes-wg-contribex" group.

To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-wg-contribex+unsub...@googlegroups.com.
To post to this group, send email to kubernetes-wg-contribex@googlegroups.com.

[Erick Fejta]

I remain unconvinced about the /approve change, as it is just busywork and adds friction to contribution.

If I own the files I am changing, creating the PR states that I approve of the PR. The primary people advocating for the change were people from a docs repo that had their OWNERS files misconfigured.

Squashing -- would there be a way to tell the bot not to squash?

To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-wg-con...@googlegroups.com.
To post to this group, send email to kubernetes-...@googlegroups.com.

To view this discussion on the web visit

https://groups.google.com/d/msgid/kubernetes-wg-contribex/c18eadf4-c853-47f9-b91a-69d864e3ed55%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "kubernetes-wg-contribex" group.

To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-wg-con...@googlegroups.com.
To post to this group, send email to kubernetes-...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-wg-contribex/60F0606C-69E3-4B06-BB9B-D1A00A7EEFB3%40vmware.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "kubernetes-wg-contribex" group.

To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-wg-con...@googlegroups.com.
To post to this group, send email to kubernetes-...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-wg-contribex/CADx2oGGdcO4EjdOrPk1PWMO%3DZZz8U6WAHEaH8Txz1vNhg62-Lw%40mail.gmail.com.

[Timothy Pepper]

I really like the /approve change, unrelated to the docs flow and as described a while back on the dev list.  This change encourages a review and approve workflow that is more consistent across contributors.  Certainly for owner maintainers it can be viewed as friction, but in the long run that should encourage areas to have more reviewers, more active reviewers, and higher velocity.  Which is a big deal and not just busy work.  I mean yes I implicitly think I approve of my non-WIP PR’s as proposed, but sometimes less so as soon as they’re reviewed and I facepalm myself and want to remove my implicit approval for a while.

 

-- 

Tim Pepper

Open Source Cloud Technologies Lead

VMware Open Source Technology Center

[Joe Beda]

My 2 cents:

• I'm cool with the approve change as the author can still manually self-approve.
• I'm less convinced on the squash change. I often create PRs with logical sets of commits.  But I know that many users don't do that.  What I'd love to see is support for "autosquash".  This only squashes the commits with "fixup" in the description.  But, alas, it doesn't look like github supports this. To do this we'd have to have systems rewrite the commit. Boo. More thoughts here:
• We should consider "rebase and merge" too.  That at least keeps the history linear.
• Doing anything beyond regular merge will invalidate/remove GPG signatures.
• My summary: there are no good answers here. I'm not convinced that squashing is the right answer in *all* situations. I'd love a way to tell the system what to do with squash as a default, perhaps.

--

You received this message because you are subscribed to the Google Groups "kubernetes-wg-contribex" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-wg-con...@googlegroups.com.
To post to this group, send email to kubernetes-...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-wg-contribex/129E775E-8BCE-4524-BB3D-9715FB693885%40vmware.com.

[Erick Fejta]

It is not clear to me how making me add an /approve comment to my PRs encourages me to find more reviewers for my code.

Yes, the original/current intent of the system is that being an approver for file X means you own file X. And if you do not own file X you should not be an approver for X.

This change seems irrelevant to newcomers (they do not own any files). I suspect it will be a nuisance for experienced members, just like /approve no-issue (I would expect top-level owners of a repo to understand who needs to review their PR and /assign it to them).

On Tue, Mar 20, 2018 at 12:08 PM Christoph Blecker <cble...@gmail.com> wrote:

Being an approver doesn't always mean you "own" the files. For example, Jorge/myself/Caleb/Ihor.. we're top-level owners for k/community. This is to approve documentation changes in root, sigs.yaml, etc. That doesn't mean I own every kep or design proposal, and I would absolutely want someone who actually owns those to do the approving.

There is the anti-pattern of "/approve cancel" after the PR is opened, but that seems way less intuitive than self-approving either right away, or after LGTM. I'd rather forget to approve my own PR and it take longer to merge, then something unintended merging. I think k/community is a good candidate to try this workflow out, as there is lots of different types of content there with lots of different sigs owning.

[Timothy Pepper]

I’m coming from experience in other projects where owners of files or subsystems or repos as a matter of culture do not normally approve and merge their own work without first getting additional peer review.  I think this is a health practice.  It still allows for pragmatic and fast merging when needed, but hopefully with an “/approve because things are on fire, others aren’t around to review immediately and I think this ought to fix it and we’ll deal with it if not”.

 

But the idea is that’s not the normal path, compared to 1) propose PR, 2) get peer review, 3) declare consensus and approve for merge automation to take over.  Which is a relatively normal workflow elsewhere in software engineering.  And thus why I see this relating to newcomers.  Newcomers (as well as the more senior folks who’ve noted the oddity as they’re in higher level OWNERS files) are likely to expect this workflow and are surprised and confused by the magic stateful approval semantics of /lgtm.  I regularly hear newcomers confused by some PRs sitting around for a long time even with positive review and other parallel PRs flying through instantly with no review.  People perceive there’s some special in-crowd who they must not be managing to break into as an unwelcome outsider, which is unhealthy.  I believe consistent, intuitive process eases community growth at both the new contributor and new reviewer levels.  There’s not a parallel magic track for blessed individuals PRs.

 

I honestly wish the two commands were something more like /reviewed{-ok,changes-required} and /merge.  And then I imagine you’d only be doing /merge.

 

-- 

Tim Pepper

Open Source Cloud Technologies Lead

VMware Open Source Technology Center

From: 'Erick Fejta' via kubernetes-wg-contribex <kubernetes-...@googlegroups.com>

Reply-To: Erick Fejta <fe...@google.com>
Date: Tuesday, March 20, 2018 at 1:06 PM
To: Christoph Blecker <cble...@gmail.com>
Cc: Garrett Rodrigues <gr...@google.com>, Timothy Pepper <tpe...@vmware.com>, kubernetes-wg-contribex <kubernetes-...@googlegroups.com>

Subject: Re: Bot configuration change for k/community repo

[Christoph Blecker]

Being an approver doesn't always mean you "own" the files. For example, Jorge/myself/Caleb/Ihor.. we're top-level owners for k/community. This is to approve documentation changes in root, sigs.yaml, etc. That doesn't mean I own every kep or design proposal, and I would absolutely want someone who actually owns those to do the approving.

There is the anti-pattern of "/approve cancel" after the PR is opened, but that seems way less intuitive than self-approving either right away, or after LGTM. I'd rather forget to approve my own PR and it take longer to merge, then something unintended merging. I think k/community is a good candidate to try this workflow out, as there is lots of different types of content there with lots of different sigs owning.

On 20 March 2018 at 11:56, Erick Fejta <fe...@google.com> wrote:

To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-wg-contribex+unsub...@googlegroups.com.
To post to this group, send email to kubernetes-wg-contribex@googlegroups.com.

To view this discussion on the web visit

https://groups.google.com/d/msgid/kubernetes-wg-contribex/c18eadf4-c853-47f9-b91a-69d864e3ed55%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "kubernetes-wg-contribex" group.

To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-wg-contribex+unsub...@googlegroups.com.
To post to this group, send email to kubernetes-wg-contribex@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-wg-contribex/60F0606C-69E3-4B06-BB9B-D1A00A7EEFB3%40vmware.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "kubernetes-wg-contribex" group.

To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-wg-contribex+unsub...@googlegroups.com.
To post to this group, send email to kubernetes-wg-contribex@googlegroups.com.

[Derek Carr]

i prefer that we let authors self-approve by default to avoid a redundant comment.  for the times where i do not want automatic approval, i have been fine labeling it do not merge.

To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-wg-contribex+unsub...@googlegroups.com.
To post to this group, send email to kubernetes-wg-contribex@googlegroups.com.

To view this discussion on the web visit

https://groups.google.com/d/msgid/kubernetes-wg-contribex/c18eadf4-c853-47f9-b91a-69d864e3ed55%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "kubernetes-wg-contribex" group.

To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-wg-contribex+unsub...@googlegroups.com.
To post to this group, send email to kubernetes-wg-contribex@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-wg-contribex/60F0606C-69E3-4B06-BB9B-D1A00A7EEFB3%40vmware.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "kubernetes-wg-contribex" group.

To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-wg-contribex+unsub...@googlegroups.com.
To post to this group, send email to kubernetes-wg-contribex@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-wg-contribex/CADx2oGGdcO4EjdOrPk1PWMO%3DZZz8U6WAHEaH8Txz1vNhg62-Lw%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "kubernetes-wg-contribex" group.

To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-wg-contribex+unsub...@googlegroups.com.
To post to this group, send email to kubernetes-wg-contribex@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-wg-contribex/CAMMDcuEiy%2BdxomVHdAtkzRTOjhwqNTojt61OrmgXKxJ5dwq8%3Dw%40mail.gmail.com.

[Christoph Blecker]

Hi everyone,

Based on the feedback here as well as in last week's ContribEx call, I propose the following to move forward:

- We proceed with disabling the implicit self-approve for k/community *only*, and solicit feedback. We will monitor PRs to see how they progress through the process. In approx 1 quarter (basically the 1.11 release cycle), we will review and see where we are at. Based on that feedback cycle, we can decide if we should roll this change out wider as the new default, or if we should restore the previous behaviour.

- We will not proceed with changing the default merge method to squash at this time. While it may help in some cases with newer contributors, there are some downsides to applying it across the board. I would like to explore Ryan's idea around a label/command to change the merge method on a per-PR basis in tide. This would allow us the advantages of squashing some PRs, but not force it on all PRs in a repo by default.

While there was some feedback received that is against the idea of disabling the implicit self-approve, we are looking to test out the "principle of least astonishment" here, and let authors be explicit with the desires.

For clarity: If you are an approver in an OWNERS file, you are still permitted to approve your own PRs. That will not change. What this changes is that you need to explicitly sign off as an approver (with a /approve), that you do approve and wish to use your powers. An LGTM from a non-author will also still be required to merge.

Last call for feedback!

Cheers,

Christoph

--