A quick poll was had on the dev channel just now about how to apply
the "Don't merge your own PRs" rule to the larger branches created by
feature teams. It was only a brief discussion, but I thought I'd
summarize here in case anyone missed it ahs has objections.
The problem is that features are typically large PRs, which are often
left unreviewed for considerable time. This is usually due to the fact
that it's difficult for devs outside the core feature team to find
enough time to really set up and test something that big.
The feeling was that while (of course) it's best to have a full
review, but if all members of the feature team ACK the PR, and a
suitable merge deadline is given to allow other interested devs a
chance to review, then it's ok for the feature team to merge their
work. For "suitable deadline", it was suggested that a week would be
reasonable if the PR has already been up for general review for a
while, but should take account of events that would impact the number
of people being around to look at the code (Christmas, FOSDEM, etc).
···
---
Assuming no-one has a problem with the above process, I’d like to give
a one week notice for the new networking features (PR #1988). It’s
been open for a month, I use it daily with no major issues, and by
merging it soon we can iron out any edge cases before 1.8 is cut.
Therefore, assuming no objections are raised, we will merge it next
week, on Tues 13th.
No major issues from my side with this kind of policy, however I think it's
important to still keep the rule 'do not merge your own PR'. Either please
someone from the feature team who have thoroughly tested it or someone in
core familiar with it should merge it. In addition to that, I think a 10
days deadline for at least a high level pass from any core reviewers should
be required to merge something that big. If there are any issues with the
PR, extend the deadline for another 10 days. I don't know how to keep core
reviewers accountable in case they miss these deadlines, for myself I'm OK
if you ping me on IRC constantly if I look like I forgot about some PR.
I disagree that biting the bullet for these kind of large features and then
ironing out the bugs is a good approach, I'm not against doing it in
certain cases (when deadlines are tight…) but in general I don't think
it's unreasonable to ask for splitted up work so that PRs are easier to
review. I think the trade off (more development speed, lower quality code -
or at least less tested) is difficult to justify.
Last but not least please leave a comment in these PRs about the deadline
and mention other reviewers.
Any other points of view HIGHLY appreciated!
···
On Tue, Jan 6, 2015 at 11:45 AM, Greg Sutcliffe wrote:
Hi all,
A quick poll was had on the dev channel just now about how to apply
the “Don’t merge your own PRs” rule to the larger branches created by
feature teams. It was only a brief discussion, but I thought I’d
summarize here in case anyone missed it ahs has objections.
The problem is that features are typically large PRs, which are often
left unreviewed for considerable time. This is usually due to the fact
that it’s difficult for devs outside the core feature team to find
enough time to really set up and test something that big.
The feeling was that while (of course) it’s best to have a full
review, but if all members of the feature team ACK the PR, and a
suitable merge deadline is given to allow other interested devs a
chance to review, then it’s ok for the feature team to merge their
work. For “suitable deadline”, it was suggested that a week would be
reasonable if the PR has already been up for general review for a
while, but should take account of events that would impact the number
of people being around to look at the code (Christmas, FOSDEM, etc).
Assuming no-one has a problem with the above process, I’d like to give
a one week notice for the new networking features (PR #1988). It’s
been open for a month, I use it daily with no major issues, and by
merging it soon we can iron out any edge cases before 1.8 is cut.
Therefore, assuming no objections are raised, we will merge it next
week, on Tues 13th.
> work. For "suitable deadline", it was suggested that a week would be
I agree with Greg and I would suggest us to do the same "suitable
deadline" of a week from an announcement on the dev mailing list.
I was rolling this particular one in my TODO until Greg raised this and
this was the trigger for me to take a look. I think it is not possible
to follow all the github conversations for us and a nice little friendly
reminder is a huge help.
I agree with Greg's viewpoint that PR's should be able to be merged if the feature team agrees. Just for proper procedure though, do "all" or "most" or a "at least 3" members of a feature team need to ACK a PR on github. This will make it easier to follow procedure for those with merge rights.
Joseph
···
----- Original Message -----
From: “Daniel Lobato” elobatocs@gmail.com
To: foreman-dev@googlegroups.com
Sent: Wednesday, January 7, 2015 3:00:46 PM
Subject: Re: [foreman-dev] How to merge Features? (and: networking feature
merge heads-up)
No major issues from my side with this kind of policy, however I think it’s
important to still keep the rule ‘do not merge your own PR’. Either please
someone from the feature team who have thoroughly tested it or someone in
core familiar with it should merge it. In addition to that, I think a 10
days deadline for at least a high level pass from any core reviewers should
be required to merge something that big. If there are any issues with the
PR, extend the deadline for another 10 days. I don’t know how to keep core
reviewers accountable in case they miss these deadlines, for myself I’m OK
if you ping me on IRC constantly if I look like I forgot about some PR.
I disagree that biting the bullet for these kind of large features and then
ironing out the bugs is a good approach, I’m not against doing it in certain
cases (when deadlines are tight…) but in general I don’t think it’s
unreasonable to ask for splitted up work so that PRs are easier to review. I
think the trade off (more development speed, lower quality code - or at
least less tested) is difficult to justify.
Last but not least please leave a comment in these PRs about the deadline and
mention other reviewers.
+1 for feature team merging their own PRs. "Do not merge your own commit" is
rule not broken if PR consists of two commits from different authors and they
are cross-checked (I hope it's not just icehockey term and fully reviewed
and tested.
···
--
Marek
On Wednesday 07 of January 2015 14:00:46 Daniel Lobato wrote:
No major issues from my side with this kind of policy, however I think it’s
important to still keep the rule ‘do not merge your own PR’. Either please
someone from the feature team who have thoroughly tested it or someone in
core familiar with it should merge it. In addition to that, I think a 10
days deadline for at least a high level pass from any core reviewers should
be required to merge something that big. If there are any issues with the
PR, extend the deadline for another 10 days. I don’t know how to keep core
reviewers accountable in case they miss these deadlines, for myself I’m OK
if you ping me on IRC constantly if I look like I forgot about some PR.
I disagree that biting the bullet for these kind of large features and then
ironing out the bugs is a good approach, I’m not against doing it in
certain cases (when deadlines are tight…) but in general I don’t think
it’s unreasonable to ask for splitted up work so that PRs are easier to
review. I think the trade off (more development speed, lower quality code -
or at least less tested) is difficult to justify.
Last but not least please leave a comment in these PRs about the deadline
and mention other reviewers.
A quick poll was had on the dev channel just now about how to apply
the “Don’t merge your own PRs” rule to the larger branches created by
feature teams. It was only a brief discussion, but I thought I’d
summarize here in case anyone missed it ahs has objections.
The problem is that features are typically large PRs, which are often
left unreviewed for considerable time. This is usually due to the fact
that it’s difficult for devs outside the core feature team to find
enough time to really set up and test something that big.
The feeling was that while (of course) it’s best to have a full
review, but if all members of the feature team ACK the PR, and a
suitable merge deadline is given to allow other interested devs a
chance to review, then it’s ok for the feature team to merge their
work. For “suitable deadline”, it was suggested that a week would be
reasonable if the PR has already been up for general review for a
while, but should take account of events that would impact the number
of people being around to look at the code (Christmas, FOSDEM, etc).
Assuming no-one has a problem with the above process, I’d like to give
a one week notice for the new networking features (PR #1988). It’s
been open for a month, I use it daily with no major issues, and by
merging it soon we can iron out any edge cases before 1.8 is cut.
Therefore, assuming no objections are raised, we will merge it next
week, on Tues 13th.
Maybe the label "ready for merge" can be set after the feature team
approves the PR with the deadline in it?
I haven't seen it widely used.
That could help get people to notice they need to take a look now or the PR
will be merged soon.
The only problem I see with this is if there are no core committers in the
feature team.
In that case the approval of all feature team members might not be enough
to merge.
···
On Wed, Jan 7, 2015 at 4:09 PM, Lukas Zapletal wrote:
work. For “suitable deadline”, it was suggested that a week would be
I agree with Greg and I would suggest us to do the same “suitable
deadline” of a week from an announcement on the dev mailing list.
I was rolling this particular one in my TODO until Greg raised this and
this was the trigger for me to take a look. I think it is not possible
to follow all the github conversations for us and a nice little friendly
reminder is a huge help.
I generally agree, and would add that deadlines are possibly the worst
reason to merge something. Previous times that's left us with
half-baked, incomplete code and subsequent issues, because maintainers
are being put under pressure to merge unfinished work immediately before
releases.
Regarding the original proposal itself, I have no further opinion.
Related to this thread is that there has been a disappointing trend
recently for features to be discussed only with a small group of people,
without discussion or summing up on this list. This only serves to make
it harder for other developers who weren't part of the conversation to
review a proposed change, or even know what's going on. Please remember
to be more open by default.
···
On 07/01/15 13:00, Daniel Lobato wrote:
> I disagree that biting the bullet for these kind of large features and
> then ironing out the bugs is a good approach, I'm not against doing it
> in certain cases (when deadlines are tight...) but in general I don't
> think it's unreasonable to ask for splitted up work so that PRs are
> easier to review. I think the trade off (more development speed, lower
> quality code - or at least less tested) is difficult to justify.
I agree with that, and would always want to deliver the minimum viable
change to keep it easy for reviewers (as an example, the api changes
for the netowrking are not in our PR, since it's backwards compatible
with the old API for now). However, it's not always possible to
deliver something that works in a smaller PR (for previous examples,
consider the changes to the permissions system that was merged around
this time last year). Sometimes a change is necessarily huge.
···
On 7 January 2015 at 13:00, Daniel Lobato wrote:
> I disagree that biting the bullet for these kind of large features and then
> ironing out the bugs is a good approach, I'm not against doing it in certain
> cases (when deadlines are tight...) but in general I don't think it's
> unreasonable to ask for splitted up work so that PRs are easier to review. I
> think the trade off (more development speed, lower quality code - or at
> least less tested) is difficult to justify.
+1. Of course we all know the team is responsible and I don't think
anybody would want to misuse the fact, that this would be allowed out of sudden,
turning the code base to piecis. Especially when the branch is in the
phase where all the feature team members are confident about the code.
The Foreman team is really great about keeping things stable in the develop branch,
but it's better to merge sooner to have some time to tune up the details while
more devel users start using that as part of their work, as opposed having
the code passively waiting in the PR until the deadline approaches and driving
it directly to the release.
I think the form of "if nobody objects, we will merge this in a week" is a fair one.
– Ivan
···
----- Original Message -----
> Maybe the label "ready for merge" can be set after the feature team
> approves the PR with the deadline in it?
> I haven't seen it widely used.
> That could help get people to notice they need to take a look now or the PR
> will be merged soon.
>
> The only problem I see with this is if there are no core committers in the
> feature team.
> In that case the approval of all feature team members might not be enough
> to merge.
>
> On Wed, Jan 7, 2015 at 4:09 PM, Lukas Zapletal wrote:
>
> > > work. For "suitable deadline", it was suggested that a week would be
> >
> > I agree with Greg and I would suggest us to do the same "suitable
> > deadline" of a week from an announcement on the dev mailing list.
> >
> > I was rolling this particular one in my TODO until Greg raised this and
> > this was the trigger for me to take a look. I think it is not possible
> > to follow all the github conversations for us and a nice little friendly
> > reminder is a *huge* help.
> >
> > --
> > Later,
> > Lukas #lzap Zapletal
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "foreman-dev" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> > email to foreman-dev+unsubscribe@googlegroups.com.
> > For more options, visit https://groups.google.com/d/optout.
> >
>
> --
> You received this message because you are subscribed to the Google Groups
> "foreman-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to foreman-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>
+1. Of course we all know the team is responsible and I don’t think
anybody would want to misuse the fact, that this would be allowed out of
sudden,
turning the code base to piecis. Especially when the branch is in the
phase where all the feature team members are confident about the code.
The Foreman team is really great about keeping things stable in the
develop branch,
but it’s better to merge sooner to have some time to tune up the details
while
more devel users start using that as part of their work, as opposed having
the code passively waiting in the PR until the deadline approaches and
driving
it directly to the release.
I think the form of “if nobody objects, we will merge this in a week” is a
fair one.
– Ivan
----- Original Message -----
Maybe the label “ready for merge” can be set after the feature team
approves the PR with the deadline in it?
I haven’t seen it widely used.
That could help get people to notice they need to take a look now or the
PR
will be merged soon.
The only problem I see with this is if there are no core committers in
the
feature team.
In that case the approval of all feature team members might not be enough
to merge.
On Wed, Jan 7, 2015 at 4:09 PM, Lukas Zapletal lzap@redhat.com wrote:
work. For “suitable deadline”, it was suggested that a week would be
I agree with Greg and I would suggest us to do the same “suitable
deadline” of a week from an announcement on the dev mailing list.
I was rolling this particular one in my TODO until Greg raised this and
this was the trigger for me to take a look. I think it is not possible
to follow all the github conversations for us and a nice little
friendly
reminder is a huge help.
–
Later,
Lukas #lzap Zapletal
–
You received this message because you are subscribed to the Google
Groups
“foreman-dev” group.
To unsubscribe from this group and stop receiving emails from it, send
an
> +1. Of course we all know the team is responsible and I don't think
> anybody would want to misuse the fact, that this would be allowed out of sudden,
> turning the code base to piecis. Especially when the branch is in the
> phase where all the feature team members are confident about the code.
+1
···
On 01/07/2015 05:42 PM, Ivan Necas wrote:
The Foreman team is really great about keeping things stable in the develop branch,
but it’s better to merge sooner to have some time to tune up the details while
more devel users start using that as part of their work, as opposed having
the code passively waiting in the PR until the deadline approaches and driving
it directly to the release.
I think the form of “if nobody objects, we will merge this in a week” is a fair one.
– Ivan
----- Original Message -----
Maybe the label “ready for merge” can be set after the feature team
approves the PR with the deadline in it?
I haven’t seen it widely used.
That could help get people to notice they need to take a look now or the PR
will be merged soon.
The only problem I see with this is if there are no core committers in the
feature team.
In that case the approval of all feature team members might not be enough
to merge.
On Wed, Jan 7, 2015 at 4:09 PM, Lukas Zapletal lzap@redhat.com wrote:
work. For “suitable deadline”, it was suggested that a week would be
I agree with Greg and I would suggest us to do the same “suitable
deadline” of a week from an announcement on the dev mailing list.
I was rolling this particular one in my TODO until Greg raised this and
this was the trigger for me to take a look. I think it is not possible
to follow all the github conversations for us and a nice little friendly
reminder is a huge help.
Whilst I agree with both of those comments, in particular regard to
the feature I'm trying to get merged, we've sent a summary to the
list, done deep dives, asked repeatedly for reviews, and tried to
respond to what limited response we got in good time. We're holding
our end up, I feel. It was only after I sent this mail and proposed a
merge next week that we suddenly got more review comments. Whilst
you're right about that trend, if the rest of the merge process is
broken when devs are doing it right, then we've no hope of
convincing them to continue to do it right.
I agree about deadlines being arbitrary, but I can't think of any
other way to stop large-but-useful PRs from succumbing to bitrot hell.
In particular, if I wrote the entire feature and Marek reviewed it, no
one would have an issue. But because we've written it together, and
reviewed each other's work (and added Tomas in for good measure) it's
now an issue? The deadline for wider reviews could be seen as
extraneous under that viewpoint, but I felt it was a courtesy extended
to the wider team (in our case the proposed deadline will now be
pushed back until after we've dealt with Lukas' comments).
If you've got better ideas, then I'm eager to hear them, but we cannot
continue to allow big chunks of work to waste away when the people who
wrote them have been pushing for reviews for over a month.
Greg
···
On 8 January 2015 at 08:30, Dominic Cleal wrote:
> On 07/01/15 13:00, Daniel Lobato wrote:
>> I disagree that biting the bullet for these kind of large features and
>> then ironing out the bugs is a good approach, I'm not against doing it
>> in certain cases (when deadlines are tight...) but in general I don't
>> think it's unreasonable to ask for splitted up work so that PRs are
>> easier to review. I think the trade off (more development speed, lower
>> quality code - or at least less tested) is difficult to justify.
>
> I generally agree, and would add that deadlines are possibly the worst
> reason to merge something. Previous times that's left us with
> half-baked, incomplete code and subsequent issues, because maintainers
> are being put under pressure to merge unfinished work immediately before
> releases.
>
> Regarding the original proposal itself, I have no further opinion.
>
> Related to this thread is that there has been a disappointing trend
> recently for features to be discussed only with a small group of people,
> without discussion or summing up on this list. This only serves to make
> it harder for other developers who weren't part of the conversation to
> review a proposed change, or even know what's going on. Please remember
> to be more open by default.
> If you've got better ideas, then I'm eager to hear them, but we cannot
> continue to allow big chunks of work to waste away when the people who
> wrote them have been pushing for reviews for over a month.
I think Dom did not mean your feature specifically. You set the standard
actually
What I want to add to this discussion once again is that I really like
plain email instead of labels, github comment (that gets lost in my
notification everyday-storm) or any other mean.
and fully reviewed