Hi,
I've just notices fair amount of merged PR's having still 'Needs testing' and 'Not yet reviewed'
labels on it. So the little question popped up in my head: why are we doing this [1]
Maybe it's worth a small retrospective the usefulness of using the labels the way we do.
Is there really some process build around them?
[1] - http://www.phdcomics.com/comics/archive.php?comicid=3
– Ivan
A lot of the merged PRs I see with 'Needs testing' and 'Not yet reviewed' labels were simply ACKed with no review comments/test failures so I think that's ok. I think the labels are really only useful if in cases where you test and/or review the PR, and find something wrong. Then you can remove the appropriate label and add "Waiting on Contributor". That way we know if it's tested and/or reviewed already.
David
'Needs testing' and 'Not yet reviewed' are labels automatically added
to PRs by the Foreman prprocessor
(https://github.com/theforeman/prprocessor/). They help us spot PRs
who have not been opened yet, as opposed to updated PRs, where the
label is 'Needs re-review'.
Normally reviewers remove them (or just 'Not yet reviewed' if the PR
was not tested) and set 'Waiting on contributor' when the code needs
changes. After the contributor updates the PR, prprocessor adds the
labels 'Needs re-review' 'Needs testing' and the process is repeated
until the PR is deemed ready.
Like David said previously, only PRs that were ready without having to
ever update it are normally merged with these labels. The reason is
(in my case at least) that I find it pointless to remove the labels
for a PR I'm going to merge right away. The "Not yet reviewed" label
is useful even if you don't find anything wrong and merge the PR, as
it allows you to filter and see if there's anyone whose work hasn't
been reviewed at all.
–
Daniel Lobato
@elobatoss
blog.daniellobato.me
daniellobato.me
GPG: http://keys.gnupg.net/pks/lookup?op=get&search=0x7A92D6DD38D6DE30
> 'Needs testing' and 'Not yet reviewed' are labels automatically added
> to PRs by the Foreman prprocessor
> (https://github.com/theforeman/prprocessor/). They help us spot PRs
> who have not been opened yet, as opposed to updated PRs, where the
> label is 'Needs re-review'.
>
> Normally reviewers remove them (or just 'Not yet reviewed' if the PR
> was not tested) and set 'Waiting on contributor' when the code needs
> changes. After the contributor updates the PR, prprocessor adds the
> labels 'Needs re-review' 'Needs testing' and the process is repeated
> until the PR is deemed ready.
>
> Like David said previously, only PRs that were ready without having to
> ever update it are normally merged with these labels. The reason is
> (in my case at least) that I find it pointless to remove the labels
> for a PR I'm going to merge right away. The "Not yet reviewed" label
> is useful even if you don't find anything wrong and merge the PR, as
> it allows you to filter and see if there's anyone whose work hasn't
> been reviewed at all.
Thanks for explanation.
TBH, I find it poinless to remove the 'Not yet reviews' and 'Needs testing'
labels in all the other cases as I don't think we would need the initial 'needs testing'
and 'not yet reviewed' labels to be there, as almost all of the PRs have them set, which
make them useless for filtering. 'Waiting on contributor' and 'Needs re-review' are fine
(those are also set manually), even the needs-testing in case somebody reviewed that, but
still more testing is required.
Anyway, I don't feel that strong about that to drive the change. Just wanted to
have others opinion and share mine on this. If you're happy, I'm happy (well… in terms
of I will not complain publicly about that anymore 
– Ivan
On Thu, Jan 8, 2015 at 9:40 PM, David Davis daviddavis@redhat.com wrote:
A lot of the merged PRs I see with ‘Needs testing’ and 'Not yet reviewed’
labels were simply ACKed with no review comments/test failures so I think
that’s ok. I think the labels are really only useful if in cases where you
test and/or review the PR, and find something wrong. Then you can remove
the appropriate label and add “Waiting on Contributor”. That way we know
if it’s tested and/or reviewed already.David
----- Original Message -----
From: “Ivan Necas” inecas@redhat.com
To: “foreman-dev” foreman-dev@googlegroups.com
Sent: Thursday, January 8, 2015 3:34:22 PM
Subject: [foreman-dev] Github labels messHi,
I’ve just notices fair amount of merged PR’s having still 'Needs testing’
and
’Not yet reviewed’
labels on it. So the little question popped up in my head: why are we
doing
this [1]Maybe it’s worth a small retrospective the usefulness of using the labels
the
way we do.
Is there really some process build around them?[1] - http://www.phdcomics.com/comics/archive.php?comicid=3
– Ivan
–
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.–
Daniel Lobato@elobatoss
blog.daniellobato.me
daniellobato.meGPG: http://keys.gnupg.net/pks/lookup?op=get&search=0x7A92D6DD38D6DE30
–
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.
We don't set the "Not yet reviewed" "Needs testing" labels manually,
so the only point we have to take action is when replacing them, which
is only 2 clicks.
They also let the contributor now the state of the PR, but in any case
what do you mean with 'almost all of the PRs have them set'? 24/64 PRs
have "Not yet reviewed". Most PRs have "Needs testing" until they are
merged though, that one is not useful for filtering. I find it useful
every now and then when I see a PR "Waiting on contributor" and "Needs
testing", I can test the feature before the new code comes in.
I'm happy with labels, not sure about others
–
Daniel Lobato
@elobatoss
blog.daniellobato.me
daniellobato.me
GPG: http://keys.gnupg.net/pks/lookup?op=get&search=0x7A92D6DD38D6DE30
> We don't set the "Not yet reviewed" "Needs testing" labels manually,
> so the only point we have to take action is when replacing them, which
> is only 2 clicks.
>
> They also let the contributor now the state of the PR, but in any case
> what do you mean with 'almost all of the PRs have them set'? 24/64 PRs
> have "Not yet reviewed". Most PRs have "Needs testing" until they are
I've looked at them in general (opened and closed), where the ratio is different
(I have no exact numbers, as we started with the labels at some point, so it would
not be representative anyway). That's where my impression about the mess came from:
when you look at the merged PRs with labels 'not yet reviewed' and 'needs testing',
the impression is we merge unreviewed and non tested PRs (which I know is not true).
Thanks all for your options.
– Ivan
merged though, that one is not useful for filtering. I find it useful
every now and then when I see a PR “Waiting on contributor” and “Needs
testing”, I can test the feature before the new code comes in.I’m happy with labels, not sure about others
On Fri, Jan 9, 2015 at 9:42 AM, Ivan Necas inecas@redhat.com wrote:
----- Original Message -----
‘Needs testing’ and ‘Not yet reviewed’ are labels automatically added
to PRs by the Foreman prprocessor
(https://github.com/theforeman/prprocessor/). They help us spot PRs
who have not been opened yet, as opposed to updated PRs, where the
label is ‘Needs re-review’.Normally reviewers remove them (or just ‘Not yet reviewed’ if the PR
was not tested) and set ‘Waiting on contributor’ when the code needs
changes. After the contributor updates the PR, prprocessor adds the
labels ‘Needs re-review’ ‘Needs testing’ and the process is repeated
until the PR is deemed ready.Like David said previously, only PRs that were ready without having to
ever update it are normally merged with these labels. The reason is
(in my case at least) that I find it pointless to remove the labels
for a PR I’m going to merge right away. The “Not yet reviewed” label
is useful even if you don’t find anything wrong and merge the PR, as
it allows you to filter and see if there’s anyone whose work hasn’t
been reviewed at all.Thanks for explanation.
TBH, I find it poinless to remove the ‘Not yet reviews’ and 'Needs testing’
labels in all the other cases as I don’t think we would need the initial
’needs testing’
and ‘not yet reviewed’ labels to be there, as almost all of the PRs have
them set, which
make them useless for filtering. ‘Waiting on contributor’ and ‘Needs
re-review’ are fine
(those are also set manually), even the needs-testing in case somebody
reviewed that, but
still more testing is required.Anyway, I don’t feel that strong about that to drive the change. Just
wanted to
have others opinion and share mine on this. If you’re happy, I’m happy
(well… in terms
of I will not complain publicly about that anymore– Ivan
On Thu, Jan 8, 2015 at 9:40 PM, David Davis daviddavis@redhat.com wrote:
A lot of the merged PRs I see with ‘Needs testing’ and 'Not yet
reviewed’
labels were simply ACKed with no review comments/test failures so I
think
that’s ok. I think the labels are really only useful if in cases where
you
test and/or review the PR, and find something wrong. Then you can remove
the appropriate label and add “Waiting on Contributor”. That way we know
if it’s tested and/or reviewed already.David
----- Original Message -----
From: “Ivan Necas” inecas@redhat.com
To: “foreman-dev” foreman-dev@googlegroups.com
Sent: Thursday, January 8, 2015 3:34:22 PM
Subject: [foreman-dev] Github labels messHi,
I’ve just notices fair amount of merged PR’s having still 'Needs
testing’
and
’Not yet reviewed’
labels on it. So the little question popped up in my head: why are we
doing
this [1]Maybe it’s worth a small retrospective the usefulness of using the
labels
the
way we do.
Is there really some process build around them?[1] - http://www.phdcomics.com/comics/archive.php?comicid=3
– Ivan
–
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.–
Daniel Lobato@elobatoss
blog.daniellobato.me
daniellobato.meGPG: http://keys.gnupg.net/pks/lookup?op=get&search=0x7A92D6DD38D6DE30
–
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.–
Daniel Lobato@elobatoss
blog.daniellobato.me
daniellobato.meGPG: http://keys.gnupg.net/pks/lookup?op=get&search=0x7A92D6DD38D6DE30
–
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.
Just fot the record, so am I - Not Yet Reviewed + sort by Least
Comments is a very helpful filter when decided what to review