@reviewbybees pull requests

[Daniel Beck]

Hi everyone,

For quite some time contributors working for CloudBees have used @reviewbybees to indicate pull requests that need to be reviewed by other CloudBees developers before work on them can be considered finished by the original author.

The current process is the result of a requirement to have other bees sign off on any proposed code changes, while at the same time allowing the plugin maintainers to comment as early as possible on proposed changes, instead of hiding a proposal while we still work out the details.

Some feedback we got on this caused us to switch from +1/-1 to 'bee' and 'bug' emoticons on GitHub, so that these mandatory reviews aren't mistaken for requests to merge the PR. Additionally, we recently added a few bot-generated messages to explain what any feedback to PRs means, and to indicate when a PR is considered finished.

How this looks like can be seen e.g. in this PR: https://github.com/jenkinsci/credentials-plugin/pull/28

Please let us know what you think about the @reviewbybees approach to code reviews. Is there anything we can improve, while still achieving the goals mentioned above?

Daniel

[Kanstantsin Shautsou]

Was this discussed/allowed on some Jenkins meeting? 

Can such actions be documented/allowed somewhere in Jenkins project documentation?

[Stephen Connolly]

To give some background.

I initially set up the reviewbybees GitHub account *for our closed source repos only*... But people pick up habits and it crept out to OSS plugins *because*:

1. it is easy to use

2. It makes it easy to find pull requests using a query

3. Other than the mention `@reviewbybees` it's low impact

Very quickly though, you just get used to appending all your pull requests with the tag.

There are side-effects of the tag and our conducting internal review in the open:

1. All those +1 votes can become intimidating to plugin maintainers. You have a pull request who's direction you are not entirely happy with and a couple of CloudBees people look to be saying: "super this should be a no brainer to merge". That is not what our review is about. We want plugin maintainers to retain control of their repositories. If they don't like a change, they should be able and free to say so. Using +1/-1 for our own internal quality process doesn't help... Hence the move to :bee: (it is review by bees after all) and :bug: (I wanted :poo: but that proposal was rejected :-( )

2. We could use the line a lot of other companies use, where we keep our changes hidden on a private fork (so our review could be hidden) and only push up once review is complete. The down side of that is that we would then be building up larger units of change "in secret" and then landing them on the plugin maintainer. It can be harder for a plugin maintainer to assert the direction they want to follow if they are facing a big piece of work that has just landed without notice at the door of your repo. Thus why we prefer to work in the open and let the plugin maintainer shout "stop that's not the direction I want" before we even finish our PR. I believe this is best for the community. Additionally the comments in the code review can help the plugin maintainer understand why we have gone for a specific design. Code review in secret deprives the maintainer of that information.

3. Some of our employees will (initially) be strangers to the community. Plugin maintainers should be given an explanation of why a bunch of strangers are littering pull requests with :bee; and :bug: comments. So we have added that a bit adds a comment explaining that we have a process and we are not going to ask for it to be merged until our process is done - but plugin maintainers can do whatever they want. 

4. Some of us have two hats. I am a plugin maintainer for some plugins and also a core committer. It may not be obvious when I am wearing each hat. To help clarify we have the bot provide a formal request for the pull request to be merged. Thus my actions after the review is complete can be more clearly differentiated. The formal request, we also believe, is good to let plugin maintainers know that our review is complete.

In saying that, we don't want to antagonise the community. We want to do self code review and we want plugin maintainers to be free to decide what contributions they accept. We value community feedback, hence this thread.

- Stephen

On Thursday, July 2, 2015, Kanstantsin Shautsou <kanstan...@gmail.com> wrote:

Was this discussed/allowed on some Jenkins meeting? 

Can such actions be documented/allowed somewhere in Jenkins project documentation?

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/69b68969-0436-40bd-a3cb-0e30424f6d12%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--
Sent from my phone

[oliver gondža]

In fact, I do not understand the motivation behind this. You care for
reviews so much that it takes several Jenkins veterans to incorporate
trivial javadoc change, though you are fine if other people merge changes
(that will eventually find its way into your product) without any review.
To me, it seems it would make more sense for CloudBees to focus on
improving overall review disciple in whole jenkinsci organization. I would
be much happier with @reviewbyjenkinscore, and I am fine considering all
bees Jenkins core contributors for reviewing purposes.

I do not object against your procedure as it is still will be an
improvement for Jenkins, though it seems odd.

--
oliver

[domi]

I think this is just fine, but I just noticed that its actually not so easy to see whether a comment is really coming from a CB developer or not. Most of the user ids used on GitHub are not associated to a CB email.

Just because you add :bug: or :bee: to a comment know one knows if this is coming from a CB employee. I think many people will actually start to use these icons just because they have seen it on any jenkins PR and think it a common thing to do in this organisation.

/Domi

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CA%2BnPnMxanAQzOaSOPy68C3wB44xyiW4bEd5j5gdbXLXnfB7-iA%40mail.gmail.com.

[Stephen Connolly]

Yes, we have a bot that tracks that (but we are still working on refining how the bot detects cloudbees users, so for now it doesn't. If we notice this becoming a problem - which we will as the bot spams our hipchat instance with nags, etc - we will switch to a hardcoded list of employees until we get a better "automatic" solution)

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/DDD148B3-BAD6-4187-84C0-3E8177C2DF33%40fortysix.ch.

[Kanstantsin Shautsou]

Is it possible not to post this description (spam) messages? If it rule for CB employers, then you can send it to employers. If maintainer will decide merge this, then he doesn't need to know CB processes. 

To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-dev+unsubscribe@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/69b68969-0436-40bd-a3cb-0e30424f6d12%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--
Sent from my phone

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CA%2BnPnMxanAQzOaSOPy68C3wB44xyiW4bEd5j5gdbXLXnfB7-iA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

[Daniel Beck]

On 02.07.2015, at 21:37, Kanstantsin Shautsou <kanstan...@gmail.com> wrote:

> Was this discussed/allowed on some Jenkins meeting?
> Can such actions be documented/allowed somewhere in Jenkins project documentation?

As this basically just adds a few comments to pull requests, which absolutely everyone can add, we didn't ask beforehand. We're of course open to discuss this with the greater community and incorporate feedback, it's why I opened this thread. If you like, we can also put this on the governance meeting agenda.

Do you have specific suggestions what we can change? If so, please tell us. For example, we could disable the automatic bot messages in specific repositories if you don't want to see them.

Note that every pull request comes with an explanation and link to the full policy included -- so wherever you see it, you can learn everything about it by clicking a link.

[Daniel Beck]

On 02.07.2015, at 22:51, oliver gondža <ogo...@gmail.com> wrote:

> In fact, I do not understand the motivation behind this. You care for reviews so much that it takes several Jenkins veterans to incorporate trivial javadoc change, though you are fine if other people merge changes (that will eventually find its way into your product) without any review. To me, it seems it would make more sense for CloudBees to focus on improving overall review disciple in whole jenkinsci organization. I would be much happier with @reviewbyjenkinscore, and I am fine considering all bees Jenkins core contributors for reviewing purposes.

We have mandatory pull requests and reviews to improve the quality of CloudBees contributions to the Jenkins community. I think this is pretty straightforward.

Regarding other developers' changes -- the Jenkins project has always been pretty permissive in what contributions it accepts, how plugin maintainership is handled, etc. This is well documented:

https://wiki.jenkins-ci.org/display/JENKINS/Governance+Document#GovernanceDocument-Lowerbarrierofentry

And while I've always had some reservations regarding 'ask and ye shall receive push access', this is (mostly) how the Jenkins project operates. As plugin maintainer, you get to decide how you develop. Any changes to this would need to go through the governance meeting and board and that's not a goal we have.

It would be great if we could manage to get some volunteers to review pull requests across the entire jenkinsci organization. But that's a different topic altogether. Feel free to contact me if you have some ideas about this.

[Ullrich Hafner]

I don’t see why it is helpful to make intermediate work visible to plug-in authors, it creates a lot of unwanted noise. I think it would be sufficient that you make code reviews within your forked repository. When the review is successful you still can open a pull request for the original repository.    

Ulli

 

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/fc6bbdbb-b4b6-4c06-9f35-7f0cbe798081%40googlegroups.com.

[Daniel Beck]

On 03.07.2015, at 16:39, Kanstantsin Shautsou <kanstan...@gmail.com> wrote:

> Is it possible not to post this description (spam) messages? If it rule for CB employers, then you can send it to employers. If maintainer will decide merge this, then he doesn't need to know CB processes.

The messages are specifically for the maintainers to explain to them what the review comments mean, and that they are free to reject or accept anything, no matter what the reviews say.

Would it help if we excluded specific repositories from these automated comments?

[Kanstantsin Shautsou]

> On Jul 3, 2015, at 17:53, Daniel Beck <m...@beckweb.net> wrote:
>
>
> On 03.07.2015, at 16:39, Kanstantsin Shautsou <kanstan...@gmail.com> wrote:
>

>> Is it possible not to post this description (spam) messages? If it rule for CB employers, then you can send it to employers. If maintainer will decide merge this, then he doesn't need to know CB processes.
>

> All of us know how this works -- the messages are specifically for the maintainers to explain to them what the review comments mean, and that they are free to reject or accept anything, no matter what the reviews say.

>
> Would it help if we excluded specific repositories from these automated comments?
>

Not sure. I’m getting this messages from all repositories that i’m tracking/participating. And i already spending time interrupting on this mail notifications.
If CB processes is not something that should change maintainer workflow, that this message is redundant.
Description in @reviewbybees repository should be enough imho.

[Jesse Glick]

On Fri, Jul 3, 2015 at 10:48 AM, Ullrich Hafner
<ullrich...@gmail.com> wrote:
> I don’t see why it is helpful to make intermediate work visible to plug-in
> authors, it creates a lot of unwanted noise. I think it would be sufficient
> that you make code reviews within your forked repository. When the review is
> successful you still can open a pull request for the original repository.

I think the presence of the `work-in-progress` label is sufficient to
distinguish whether the PR author believes the work is complete or
not. A repository maintainer might choose to completely ignore PRs
until this label is removed. But then again they might be able to see
at a glance that the approach under development is off base, and want
to intervene early. Why not keep the discussion open for anyone who
might want to give early feedback?

[Manuel Jesús Recena Soto]

I agree with Jesse.

For me, "WiP" means "hey, I'm working on it, I've split the task in
small task and I want to share the process".

Regards,

> --
> You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANfRfr3dT8XAMggoH1Rd6NiACtdzfAm26pbCKGH5dq5dprbtGA%40mail.gmail.com.

> For more options, visit https://groups.google.com/d/optout.



--

Manuel Recena Soto
* manuelrecena.com [/blog]
* linkedin.com/in/recena

[Kanstantsin Shautsou]

It was fine until CB "firstly saw jenkins" developers started doing PRs where comments become >30. 

Could you do initial PR implementation in forked repository for such cases and then fill final PR?