Updating approval rules
27 views
Skip to first unread message
Zane Bitter
May 13, 2021, 12:05:15 PM5/13/21
to Metal3 Development List
I think I brought this topic up somewhere before, but I don't remember
where so let's discuss it again :)
I think our current Prow configuration, which got that way by default
rather than any conscious decision, makes use of the /lgtm command too
high-stakes. A single /lgtm suffices to merge a PR when *either* the
author or the reviewer is an approver. The reasons for this are that an
approver submitting a PR automatically marks it as approved, and an
approver adding /lgtm to a PR also marks it approved.
Because we have an awesome and very respectful community, the main
result of that is that nobody wants to /lgtm a patch until absolutely
everyone has looked at it because they don't want to step on any toes. I
think it also contributes to a reluctance to add people as reviewers,
even though our stated policy is that basically anyone who is involved
in the project on an ongoing basis should be a reviewer.
I also believe it is wrong in principle. In an open source community,
maintainers should submit themselves to the same code review standards
as everyone else. It's part of the social contract (see the introduction
to [1] - full disclosure: which I wrote). Projects that fail to apply
this rule have a tendency to run into culture problems at scale (perhaps
you can think of a certain kernel that this might apply to).
Doug has done the hard work of figuring out the magic syntax to fix the
config (thanks Doug!), so if you agree then go here and, uh, I guess
/approve:
https://github.com/metal3-io/project-infra/pull/190
If folks agree then let's a adopt a heuristic that in general every PR
should have a /approve from an approver and a /lgtm from a separate
reviewer neither of whom are the author, but also that in general it
need not have *more* than that. (Obviously continue to use your
judgement in individual cases.) I think that by making it harder to
merge a PR, we should actually be able to speed up merging by quite a bit.
A possible exception is design proposals in metal3-docs. Currently we
don't have a clear policy for those, and the result is similarly that it
takes forever to merge stuff (often they merge after the code
implementing it). Informally we've been doing it by deadline, calling
for a lazy consensus in the community meeting. Perhaps we should
formalise that policy?
cheers,
Zane.
[1]
https://docs.openstack.org/project-team-guide/review-the-openstack-way.html
where so let's discuss it again :)
I think our current Prow configuration, which got that way by default
rather than any conscious decision, makes use of the /lgtm command too
high-stakes. A single /lgtm suffices to merge a PR when *either* the
author or the reviewer is an approver. The reasons for this are that an
approver submitting a PR automatically marks it as approved, and an
approver adding /lgtm to a PR also marks it approved.
Because we have an awesome and very respectful community, the main
result of that is that nobody wants to /lgtm a patch until absolutely
everyone has looked at it because they don't want to step on any toes. I
think it also contributes to a reluctance to add people as reviewers,
even though our stated policy is that basically anyone who is involved
in the project on an ongoing basis should be a reviewer.
I also believe it is wrong in principle. In an open source community,
maintainers should submit themselves to the same code review standards
as everyone else. It's part of the social contract (see the introduction
to [1] - full disclosure: which I wrote). Projects that fail to apply
this rule have a tendency to run into culture problems at scale (perhaps
you can think of a certain kernel that this might apply to).
Doug has done the hard work of figuring out the magic syntax to fix the
config (thanks Doug!), so if you agree then go here and, uh, I guess
/approve:
https://github.com/metal3-io/project-infra/pull/190
If folks agree then let's a adopt a heuristic that in general every PR
should have a /approve from an approver and a /lgtm from a separate
reviewer neither of whom are the author, but also that in general it
need not have *more* than that. (Obviously continue to use your
judgement in individual cases.) I think that by making it harder to
merge a PR, we should actually be able to speed up merging by quite a bit.
A possible exception is design proposals in metal3-docs. Currently we
don't have a clear policy for those, and the result is similarly that it
takes forever to merge stuff (often they merge after the code
implementing it). Informally we've been doing it by deadline, calling
for a lazy consensus in the community meeting. Perhaps we should
formalise that policy?
cheers,
Zane.
[1]
https://docs.openstack.org/project-team-guide/review-the-openstack-way.html
Maël Kimmerlin
May 14, 2021, 3:51:14 AM5/14/21
to Zane Bitter, Metal3 Development List
Hi there!
I am definitely in favour of this proposal. I would however specify a bit more the review process, specifically the order of the labels :
Considering that the "approved" label is permanent, while the "lgtm" is removed every time the PR changes, I would recommend to change the order we tend to apply it, meaning applying "lgtm" first, when a broader set of people can do it, and then "approve" only
PRs that already have "lgtm" to merge them. That puts the final responsibility on the approvers and reviewers might feel less ill-at-ease putting a lgtm, knowing that it still needs approval from one of the approvers. Until now we were sometimes putting approval
first, I think we should avoid that. That might put slightly more load on approvers, but I think it will benefit the community in the long run.
Does this make sense ?
Cheers,
Maël
From: metal...@googlegroups.com <metal...@googlegroups.com> on behalf of Zane Bitter <zbi...@redhat.com>
Sent: 13 May 2021 19:05
To: Metal3 Development List <metal...@googlegroups.com>
Subject: [metal3-dev] Updating approval rules
Sent: 13 May 2021 19:05
To: Metal3 Development List <metal...@googlegroups.com>
Subject: [metal3-dev] Updating approval rules
I think I brought this topic up somewhere before, but I don't remember
where so let's discuss it again :)
I think our current Prow configuration, which got that way by default
rather than any conscious decision, makes use of the /lgtm command too
high-stakes. A single /lgtm suffices to merge a PR when *either* the
author or the reviewer is an approver. The reasons for this are that an
approver submitting a PR automatically marks it as approved, and an
approver adding /lgtm to a PR also marks it approved.
Because we have an awesome and very respectful community, the main
result of that is that nobody wants to /lgtm a patch until absolutely
everyone has looked at it because they don't want to step on any toes. I
think it also contributes to a reluctance to add people as reviewers,
even though our stated policy is that basically anyone who is involved
in the project on an ongoing basis should be a reviewer.
I also believe it is wrong in principle. In an open source community,
maintainers should submit themselves to the same code review standards
as everyone else. It's part of the social contract (see the introduction
to [1] - full disclosure: which I wrote). Projects that fail to apply
this rule have a tendency to run into culture problems at scale (perhaps
you can think of a certain kernel that this might apply to).
Doug has done the hard work of figuring out the magic syntax to fix the
config (thanks Doug!), so if you agree then go here and, uh, I guess
/approve:
where so let's discuss it again :)
I think our current Prow configuration, which got that way by default
rather than any conscious decision, makes use of the /lgtm command too
high-stakes. A single /lgtm suffices to merge a PR when *either* the
author or the reviewer is an approver. The reasons for this are that an
approver submitting a PR automatically marks it as approved, and an
approver adding /lgtm to a PR also marks it approved.
Because we have an awesome and very respectful community, the main
result of that is that nobody wants to /lgtm a patch until absolutely
everyone has looked at it because they don't want to step on any toes. I
think it also contributes to a reluctance to add people as reviewers,
even though our stated policy is that basically anyone who is involved
in the project on an ongoing basis should be a reviewer.
I also believe it is wrong in principle. In an open source community,
maintainers should submit themselves to the same code review standards
as everyone else. It's part of the social contract (see the introduction
to [1] - full disclosure: which I wrote). Projects that fail to apply
this rule have a tendency to run into culture problems at scale (perhaps
you can think of a certain kernel that this might apply to).
Doug has done the hard work of figuring out the magic syntax to fix the
config (thanks Doug!), so if you agree then go here and, uh, I guess
/approve:
If folks agree then let's a adopt a heuristic that in general every PR
should have a /approve from an approver and a /lgtm from a separate
reviewer neither of whom are the author, but also that in general it
need not have *more* than that. (Obviously continue to use your
judgement in individual cases.) I think that by making it harder to
merge a PR, we should actually be able to speed up merging by quite a bit.
A possible exception is design proposals in metal3-docs. Currently we
don't have a clear policy for those, and the result is similarly that it
takes forever to merge stuff (often they merge after the code
implementing it). Informally we've been doing it by deadline, calling
for a lazy consensus in the community meeting. Perhaps we should
formalise that policy?
cheers,
Zane.
[1]
https://docs.openstack.org/project-team-guide/review-the-openstack-way.html
--
You received this message because you are subscribed to the Google Groups "Metal3 Development List" group.
To unsubscribe from this group and stop receiving emails from it, send an email to metal3-dev+...@googlegroups.com.
To view this discussion on the web visit https://protect2.fireeye.com/v1/url?k=d0164d51-8f8d7454-d0160dca-86959e472243-a0b0df3252220881&q=1&e=807c154c-6065-485b-bbc7-8cb54a01a031&u=https%3A%2F%2Fgroups.google.com%2Fd%2Fmsgid%2Fmetal3-dev%2F44250b3d-02c6-a1be-e83d-17213451641e%2540redhat.com.
You received this message because you are subscribed to the Google Groups "Metal3 Development List" group.
To unsubscribe from this group and stop receiving emails from it, send an email to metal3-dev+...@googlegroups.com.
To view this discussion on the web visit https://protect2.fireeye.com/v1/url?k=d0164d51-8f8d7454-d0160dca-86959e472243-a0b0df3252220881&q=1&e=807c154c-6065-485b-bbc7-8cb54a01a031&u=https%3A%2F%2Fgroups.google.com%2Fd%2Fmsgid%2Fmetal3-dev%2F44250b3d-02c6-a1be-e83d-17213451641e%2540redhat.com.
Doug Hellmann
May 14, 2021, 10:38:55 AM5/14/21
to Maël Kimmerlin, Zane Bitter, Metal3 Development List
I like Maël's idea, too. I'm not sure of the best place to write that down. Do we have review guidelines in metal3-docs somewhere?
As far as review guidelines for designs, I know that it can be frustrating to have those reviews open for a long time. I'm not sure that indicates a fundamental problem with the process, though. As a group we have a limited capacity to absorb change. As we prioritize individually, it seems natural that we're going to focus on the "urgent" items as a way of managing the rate of change of the system. Some of our oldest design reviews have been for relatively large or complex features, which implies they come with a lot of change. If there is not a collective sense of urgency behind those items, so that enough people are excited about them to help review and refine the design, I would expect them to linger. Engaging in discussions about proposals in meetings or on the mailing list is a good way to draw attention and increase the sense of urgency around a review. I would rather see us work more on encouraging those discussions.
To view this discussion on the web visit https://groups.google.com/d/msgid/metal3-dev/VI1PR0701MB2832B8311EF1BCA92BDAED39FC509%40VI1PR0701MB2832.eurprd07.prod.outlook.com.
Zane Bitter
May 14, 2021, 11:35:25 AM5/14/21
to Metal3 Development List
On 14/05/21 3:51 am, Maël Kimmerlin wrote:
> Hi there!
>
> I am definitely in favour of this proposal. I would however specify a
> bit more the review process, specifically the order of the labels :
>
> Considering that the "approved" label is permanent, while the "lgtm" is
> removed every time the PR changes, I would recommend to change the order
> we tend to apply it, meaning applying "lgtm" first, when a broader set
> of people can do it, and then "approve" only PRs that already have
> "lgtm" to merge them. That puts the final responsibility on the
> approvers and reviewers might feel less ill-at-ease putting a lgtm,
> knowing that it still needs approval from one of the approvers. Until
> now we were sometimes putting approval first, I think we should avoid
> that. That might put slightly more load on approvers, but I think it
> will benefit the community in the long run.
> Does this make sense ?
Thanks, this is a great point! I think what you're suggesting is the way
> Hi there!
>
> I am definitely in favour of this proposal. I would however specify a
> bit more the review process, specifically the order of the labels :
>
> Considering that the "approved" label is permanent, while the "lgtm" is
> removed every time the PR changes, I would recommend to change the order
> we tend to apply it, meaning applying "lgtm" first, when a broader set
> of people can do it, and then "approve" only PRs that already have
> "lgtm" to merge them. That puts the final responsibility on the
> approvers and reviewers might feel less ill-at-ease putting a lgtm,
> knowing that it still needs approval from one of the approvers. Until
> now we were sometimes putting approval first, I think we should avoid
> that. That might put slightly more load on approvers, but I think it
> will benefit the community in the long run.
> Does this make sense ?
that Prow is intended to work in the k8s community.
I think one of the reasons we've been doing /approve first is the config
- right now if you /lgtm as an approver it will insta-merge the patch.
If that's not what you wanted then /approve is the only option for
leaving a review (other than just commenting).
I sometimes use /approve first when I'm OK with the direction of a patch
but there are some minor things to clean up. That way I can unblock the
author and take myself out of the critical path by ensuring that any
reviewer can merge the PR once the fixups are done. If you see me doing
this, feel free to add /lgtm :)
I'll try to be better about not doing /approve first for other reasons
once the config change merges, and we can see how that goes.
cheers,
Zane.
Dmitry Tantsur
May 15, 2021, 8:39:22 AM5/15/21
to Zane Bitter, Metal3 Development List
It generally makes sense, but I'd like to highlight a concern that makes me uneasy about approving this proposal. We need to review more. Much more. Now we can sort-of short-cut some patches e.g. on ironic-image, with this proposal it will become impossible/undesired. Unless everyone agrees to ramp up their reviewing, we shouldn't commit to a more formal process.
Dmitry
--
You received this message because you are subscribed to the Google Groups "Metal3 Development List" group.
To unsubscribe from this group and stop receiving emails from it, send an email to metal3-dev+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/metal3-dev/44250b3d-02c6-a1be-e83d-17213451641e%40redhat.com.
--
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill
Andrea Fasano
May 20, 2021, 6:47:01 AM5/20/21
to Metal3 Development List
I'm not sure that ramping up could be a feasible option, especially in the longer term, considering that the reviewing process it's something that cannot be easily compressed (it requires time). I think that the problem is mostly related to 1) enlarging the reviewers base 2) ensure a proper reviewing coverage.
If the current proposal will make easier 1) then it'd be definitely for me. Not sure if for 2) there could be some recommended best practices we could adopt as a community to improve it.
Andrea
Zane Bitter
May 20, 2021, 11:56:37 AM5/20/21
to Metal3 Development List
On 15/05/21 8:39 am, Dmitry Tantsur wrote:
> It generally makes sense, but I'd like to highlight a concern that makes
> me uneasy about approving this proposal. We need to review more. Much
> more. Now we can sort-of short-cut some patches e.g. on ironic-image,
> with this proposal it will become impossible/undesired. Unless everyone
> agrees to ramp up their reviewing, we shouldn't commit to a more formal
> process.
I had a look back through the past couple of months of PRs to
> It generally makes sense, but I'd like to highlight a concern that makes
> me uneasy about approving this proposal. We need to review more. Much
> more. Now we can sort-of short-cut some patches e.g. on ironic-image,
> with this proposal it will become impossible/undesired. Unless everyone
> agrees to ramp up their reviewing, we shouldn't commit to a more formal
> process.
ironic-image. There are at least 2 people (Nam, Iury) who imho should be
added as reviewers at least. I found only three PRs that might have been
delayed by this.
Two were approved and LGTMd by the same person because they were
trivial. I'd expect that people would continue to use their best
judgement in context in these types of cases.
One was merged by a single LGTM because the submitter was an approver.
But it had plenty of review attention from other approvers within 1
business day (if only because it looks like the gate was stuck, heh).
Obviously I agree that more reviews would be better, but that's partly
the goal here. I'm not seeing evidence of a big potential downside.
cheers,
Zane.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/metal3-dev/44250b3d-02c6-a1be-e83d-17213451641e%40redhat.com
> <https://groups.google.com/d/msgid/metal3-dev/44250b3d-02c6-a1be-e83d-17213451641e%40redhat.com>.
> https://groups.google.com/d/msgid/metal3-dev/44250b3d-02c6-a1be-e83d-17213451641e%40redhat.com
>
>
>
> --
> Red Hat GmbH, https://de.redhat.com/ <https://de.redhat.com/> ,
Reply all
Reply to author
Forward
0 new messages