PR Merging

[Ken Finnigan]

All,

Earlier this week there was a PR [1] merged by an employee from the same company as the PR submitter, but with only one hour between the creation and merging of the PR.

As a community we need to ensure we're providing ample opportunity for other members of the community to comment and contribute to PRs, Issues, etc.

I appreciate that there are times when time might be critical, though I don't think that was an issue in this particular case. However, for the situation in which time is critical, may I suggest sending a message to the mailing list asking for quick feedback as a merge needs to be done quickly?

Personally I don't always jump on messages I get about new PRs, so we need to ensure the community has sufficient time to review and provide feedback.

Thanks

Ken

[1] - https://github.com/eclipse/microprofile-service-mesh-service-a/pull/2

[Mark Little]

Sorry I’m late to this.

I haven’t seen any update on the thread so will chime in and just echo Ken’s concerns here.

Mark.

--
You received this message because you are subscribed to the Google Groups "Eclipse MicroProfile" group.
To unsubscribe from this group and stop receiving emails from it, send an email to microprofile...@googlegroups.com.
To post to this group, send email to microp...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/microprofile/ff66c8e6-c41d-42f8-9017-34ea5cd198e9%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

---

Mark Little

mli...@redhat.com

JBoss, by Red Hat

Registered Address: Red Hat Ltd, 6700 Cork Airport Business Park, Kinsale Road, Co. Cork.
Registered in the Companies Registration Office, Parnell House, 14 Parnell Square, Dublin 1, Ireland, No.304873
Directors:Michael Cunningham (USA), Vicky Wiseman (USA), Michael O'Neill, Keith Phelan, Matt Parson (USA)

[Kevin Sutter]

Just so that this doesn't come across as just a RedHat view...  Thanks for bringing this up, Ken.  We should all be more careful and diligent on this process.  We should all try to get external reviews of material before merging.  Developers can request specific reviewers.  I know there may be cases (nits) where an external review is not necessary, but we should still allow for it.  Case in point...  My simple change proposed to change "micro release" to "patch release" caused quite the discussion.  A nit to one person may not be for another...

Thanks, Kevin

On Wednesday, July 18, 2018 at 7:30:49 AM UTC-5, Mark Little wrote:

Sorry I’m late to this.

I haven’t seen any update on the thread so will chime in and just echo Ken’s concerns here.

Mark.

On 22 Jun 2018, at 16:56, Ken Finnigan <k...@kenfinnigan.me> wrote:

All,

Earlier this week there was a PR [1] merged by an employee from the same company as the PR submitter, but with only one hour between the creation and merging of the PR.

As a community we need to ensure we're providing ample opportunity for other members of the community to comment and contribute to PRs, Issues, etc.

I appreciate that there are times when time might be critical, though I don't think that was an issue in this particular case. However, for the situation in which time is critical, may I suggest sending a message to the mailing list asking for quick feedback as a merge needs to be done quickly?

Personally I don't always jump on messages I get about new PRs, so we need to ensure the community has sufficient time to review and provide feedback.

Thanks

Ken

[1] - https://github.com/eclipse/microprofile-service-mesh-service-a/pull/2

--
You received this message because you are subscribed to the Google Groups "Eclipse MicroProfile" group.

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

To post to this group, send email to microp...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/microprofile/ff66c8e6-c41d-42f8-9017-34ea5cd198e9%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[James Roper]

+1 to everything everyone said.

On Fri, 20 Jul 2018 at 03:01, Kevin Sutter <kwsu...@gmail.com> wrote:

Just so that this doesn't come across as just a RedHat view...  Thanks for bringing this up, Ken.  We should all be more careful and diligent on this process.  We should all try to get external reviews of material before merging.  Developers can request specific reviewers.  I know there may be cases (nits) where an external review is not necessary, but we should still allow for it.  Case in point...  My simple change proposed to change "micro release" to "patch release" caused quite the discussion.  A nit to one person may not be for another...

Thanks, Kevin

On Wednesday, July 18, 2018 at 7:30:49 AM UTC-5, Mark Little wrote:

Sorry I’m late to this.

I haven’t seen any update on the thread so will chime in and just echo Ken’s concerns here.

Mark.

On 22 Jun 2018, at 16:56, Ken Finnigan <k...@kenfinnigan.me> wrote:

All,

Earlier this week there was a PR [1] merged by an employee from the same company as the PR submitter, but with only one hour between the creation and merging of the PR.

As a community we need to ensure we're providing ample opportunity for other members of the community to comment and contribute to PRs, Issues, etc.

I appreciate that there are times when time might be critical, though I don't think that was an issue in this particular case. However, for the situation in which time is critical, may I suggest sending a message to the mailing list asking for quick feedback as a merge needs to be done quickly?

Personally I don't always jump on messages I get about new PRs, so we need to ensure the community has sufficient time to review and provide feedback.

Thanks

Ken

[1] - https://github.com/eclipse/microprofile-service-mesh-service-a/pull/2

--
You received this message because you are subscribed to the Google Groups "Eclipse MicroProfile" group.

To unsubscribe from this group and stop receiving emails from it, send an email to microprofile...@googlegroups.com.

To post to this group, send email to microp...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/microprofile/ff66c8e6-c41d-42f8-9017-34ea5cd198e9%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

---

Mark Little

mli...@redhat.com

JBoss, by Red Hat

Registered Address: Red Hat Ltd, 6700 Cork Airport Business Park, Kinsale Road, Co. Cork.
Registered in the Companies Registration Office, Parnell House, 14 Parnell Square, Dublin 1, Ireland, No.304873
Directors:Michael Cunningham (USA), Vicky Wiseman (USA), Michael O'Neill, Keith Phelan, Matt Parson (USA)

--
You received this message because you are subscribed to the Google Groups "Eclipse MicroProfile" group.

To unsubscribe from this group and stop receiving emails from it, send an email to microprofile...@googlegroups.com.

To post to this group, send email to microp...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/microprofile/95365fe3-6327-4c99-bbf1-8910e3fad1f2%40googlegroups.com.

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

--

James Roper
Senior Developer, Office of the CTO

Lightbend – Build reactive apps!
Twitter: @jroper

[Ken Finnigan]

Thanks to all for the comments.

One tangential issue to this came up when I was reviewing some things on returning from PTO.

If there's been implicit, or explicit, approval of a PR within an MP meeting, then it should be reflected with that person physically approving the PR in GitHub. It could be misconstrued that a PR has been merged without external reviewers if another community member has verbally approved the PR in a meeting, but not done so on GitHub.

GitHub PRs are the official record for changes in our repositories and we need to ensure that we're dotting the i's and crossing the t's in terms of process.

Thanks

Ken

To view this discussion on the web visit https://groups.google.com/d/msgid/microprofile/CABY0rKOhb7w66N355rUq-E3ipu%3DPT8spRSaYQ7Sbc6_OM2iN7Q%40mail.gmail.com.

[James Roper]

On Fri, 27 Jul 2018 at 04:23, Ken Finnigan <k...@kenfinnigan.me> wrote:

Thanks to all for the comments.

One tangential issue to this came up when I was reviewing some things on returning from PTO.

If there's been implicit, or explicit, approval of a PR within an MP meeting, then it should be reflected with that person physically approving the PR in GitHub. It could be misconstrued that a PR has been merged without external reviewers if another community member has verbally approved the PR in a meeting, but not done so on GitHub.

In cases where this has happened in MP Reactive, I have both commented on the PR, and ensured the decision is recorded in the minutes, to say that it was agreed in the Hangout that this PR should be merged. The most important thing is to actually get the approval, the second most important thing is to ensure that anyone looking on can trace the approval. Ideally, approvals recorded directly in GitHub are the best, and I think we should aim for that, but if it's made clear in both the minutes and on the PR that approval was given in the hangout, then imo a sufficient paper trail exists to verify the approval. The alternative might mean hounding people for their tick of approval when they only just had enough time to attend the hangout, which is not ideal.

To view this discussion on the web visit https://groups.google.com/d/msgid/microprofile/CAKeeVe4HLxY8wGBUSxhmMMNkNGVjSq0d9BFphOVsUAnQmEjVWQ%40mail.gmail.com.

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

[Ken Finnigan]

On Thu, Jul 26, 2018 at 9:47 PM James Roper <ja...@lightbend.com> wrote:

On Fri, 27 Jul 2018 at 04:23, Ken Finnigan <k...@kenfinnigan.me> wrote:

Thanks to all for the comments.

One tangential issue to this came up when I was reviewing some things on returning from PTO.

If there's been implicit, or explicit, approval of a PR within an MP meeting, then it should be reflected with that person physically approving the PR in GitHub. It could be misconstrued that a PR has been merged without external reviewers if another community member has verbally approved the PR in a meeting, but not done so on GitHub.

In cases where this has happened in MP Reactive, I have both commented on the PR, and ensured the decision is recorded in the minutes, to say that it was agreed in the Hangout that this PR should be merged. The most important thing is to actually get the approval, the second most important thing is to ensure that anyone looking on can trace the approval. Ideally, approvals recorded directly in GitHub are the best, and I think we should aim for that, but if it's made clear in both the minutes and on the PR that approval was given in the hangout, then imo a sufficient paper trail exists to verify the approval. The alternative might mean hounding people for their tick of approval when they only just had enough time to attend the hangout, which is not ideal.

Great tips James, thanks.

One question, were those PRs for MP Reactive you mentioned ones that you merged and had to go back and add details of the approval, or they were merged by others? If the latter, could you link one of them as it would be great to have an example of how you're requesting the approvals are added.

 

To view this discussion on the web visit https://groups.google.com/d/msgid/microprofile/CABY0rKOFYh34pnO4-x0D2uSsPH-DB8jBHsULGHvz3JNLHDGnNA%40mail.gmail.com.

[James Roper]

On Fri, 27 Jul 2018 at 12:02, Ken Finnigan <k...@kenfinnigan.me> wrote:

On Thu, Jul 26, 2018 at 9:47 PM James Roper <ja...@lightbend.com> wrote:

On Fri, 27 Jul 2018 at 04:23, Ken Finnigan <k...@kenfinnigan.me> wrote:

Thanks to all for the comments.

One tangential issue to this came up when I was reviewing some things on returning from PTO.

If there's been implicit, or explicit, approval of a PR within an MP meeting, then it should be reflected with that person physically approving the PR in GitHub. It could be misconstrued that a PR has been merged without external reviewers if another community member has verbally approved the PR in a meeting, but not done so on GitHub.

In cases where this has happened in MP Reactive, I have both commented on the PR, and ensured the decision is recorded in the minutes, to say that it was agreed in the Hangout that this PR should be merged. The most important thing is to actually get the approval, the second most important thing is to ensure that anyone looking on can trace the approval. Ideally, approvals recorded directly in GitHub are the best, and I think we should aim for that, but if it's made clear in both the minutes and on the PR that approval was given in the hangout, then imo a sufficient paper trail exists to verify the approval. The alternative might mean hounding people for their tick of approval when they only just had enough time to attend the hangout, which is not ideal.

Great tips James, thanks.

One question, were those PRs for MP Reactive you mentioned ones that you merged and had to go back and add details of the approval, or they were merged by others? If the latter, could you link one of them as it would be great to have an example of how you're requesting the approvals are added.

Sorry, to make clear, the order was that we discussed in the Hangout, a decision was made to merge it, then I commented on the PR that we decided in the Hangout to merge it, and then merged it. There was no going back to PRs that were merged to document why in retrospect, we never had that problem. Oddly enough I'm struggling to find any where this process happened, going back through all the merged PRs in MicroProfile Reactive they were either GitHub Approved or +1 commented by at least one other vendor. I think in many cases what I'm remembering is issues, not PRs, that I closed because we decided on a resolution in the hangout, for example https://github.com/eclipse/microprofile-reactive/issues/9.

This PR (the very first after the gitter badge PR) is an example: https://github.com/eclipse/microprofile-reactive/pull/2, though it does sit as a bit of an outlier because it was right at the start of the project, it took time for other people to start contributing, so as the only contributor it was hard to gather reviews, and that PR was also an import of a significant amount of code from another location so impossible to effectively review in a single code review.

 

To view this discussion on the web visit https://groups.google.com/d/msgid/microprofile/CAKeeVe5qj9rPNQS5n2Ah1jm4pQFAyHTjim95GQV0vPCOrShStA%40mail.gmail.com.

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

[Emily Jiang]

Hi Ken,

I brought this very topic a while ago, see here.

I would like to clarify what you said. The PR your brought up was merged and it is for sample code and work in progress. Personally, I don't have problem of someone from the same company merging PRs. We are all working on github. Each company have github projects and the people reviewing each other's work from the same company. MP community is honest and I don't differentiate people based on which company they are from. Opinions matter. Honest is key to success!

We should not bring in too many road blocks to slow down by having to ask all reviews from different companies. Having that said, for any API changes or functionalities changes, the PRs should be reviewed by a couple of people from different background (e.g. different products). We all work in open source and need to be honest and do the right thing.

I think as a minimum, we should introduce a barrier to request at least one reviewer. As for how long a PR should wait, if you have comments on a PR after its merged, you can still comment and request a rework.

Thanks
Emily

[Ken Finnigan]

Emily,

In general I don't have a problem with an employee of a company merging the work of another employee of the same company. What I do have an issue with is that review/merge happening soon thereafter the PR was created, preventing anyone from the wider community an opportunity to review the work being proposed.

It shouldn't matter whether the PR contains sample code, work in progress, documentation updates, or anything else. We need to treat each PR with the same level of critique to ensure what the MicroProfile community is producing and promoting is appropriate and of good quality.

And I'm not proposing adding huge layers of process to PRs, such as requiring a half dozen approvals before merging. All I'm requesting is time for the community as a whole to review any PRs that have been submitted.

With respect to your comment about PRs being merged when comments come after the fact. Yes that's certainly possible, but isn't it preferred for at least some in the community to have a chance to review to ensure any obvious problems or issues are raised before merging?

You've also used the term Honesty quite a few times in your response. Quite frankly I'm not sure how Honesty plays any role in this situation. It's concerned with Transparency and Openness within the MicroProfile community. If employees from a single company are submitting PRs and merging them before the wider community has reviewed them, it could be perceived as aiming to benefit that one company with those changes. Whether that's the intent or not, not being transparent and open about PRs and seeking out additional reviewers could lead to skepticism about the intentions of that company.

In the end, I don't see it as unreasonable to request a day or two of allowing the community time to review and comment on a PR. And as I mentioned originally, if there's something critical that needs to be reviewed then a followup to the mailing list for action is appropriate.

Ken

--

You received this message because you are subscribed to the Google Groups "Eclipse MicroProfile" group.
To unsubscribe from this group and stop receiving emails from it, send an email to microprofile...@googlegroups.com.
To post to this group, send email to microp...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/microprofile/80e40101-2764-48fc-965d-f4f6e71e4117%40googlegroups.com.

[Emily Jiang]

You've also used the term Honesty quite a few times in your response. Quite frankly I'm not sure how Honesty plays any role in this situation. It's concerned with Transparency and Openness within the MicroProfile community. If employees from a single company are submitting PRs and merging them before the wider community has reviewed them, it could be perceived as aiming to benefit that one company with those changes. Whether that's the intent or not, not being transparent and open about PRs and seeking out additional reviewers could lead to skepticism about the intentions of that company.

We should all aim for the purpose of defining the best programming mode for microservice, not think about how to take any particular advantages for a particular app server, which is why I said honesty. Actually, in MP, my personal view is that we collaborate well. We all celebrate each other's success, as blogged by David Blevins. It is why the community is thriving.

 

In the end, I don't see it as unreasonable to request a day or two of allowing the community time to review and comment on a PR. And as I mentioned originally, if there's something critical that needs to be reviewed then a followup to the mailing list for action is appropriate.

Personally, I think any time limit for the waiting time might not be a good thing. Different PRs requires different review periods. Normally, any PRs to do with API changes takes longer to be discussed and merged. If the workgroup reaches consensus, it is time to merge, not need to wait for a fixed time. For an instance, if a build break issue, it should be merged asap after a couple of reviews. The PR merging should be managed by individual specs. PRs should be discussed on the gitter per se. Anyway, it is my view. I would like to hear how other people think.


At the moment, the more urgent issue is to place a lock on merging without being reviewed. I have brought this up in the past. I am trying to work with Eclipse infrastructure to see how a check can be added. Will report back once I have found out more.

Thanks
Emily