1:09 p.m.
Hello fellow Fedorans!
As you are probably aware, we have a rule (unspoken? Not sure if it's
formally documented or not) that pull requests in the infrastructure
group should be reviewed by another infra member before they are
merged.
This is a sound policy in my mind, but I've been thinking about a
problem that I've observed that I don't have a solution to. When I
review pull requests that other people write against Bodhi, I am able
to give much higher quality feedback than I am when I review pull
requests on our other projects, simply because I spend a lot of time in
the Bodhi code and am aware of how a lot of it works. For example, I
might suggest "hey, there's a function over in bodhi.server.util that
does what you are doing here - why not use that instead?". However,
when I review code in other projects I am much more limited since I
don't know the code for all of our projects very deeply. Mostly, I can
only make generic comments, or maybe offer some Python hints here and
there.
I don't actually have any proposal of what should change here. I think
reviewing code is good, and I think we should keep doing it. I just
wanted to share an observation I've made when I review code. If anyone
has any ideas on how we could improve this I'd love to hear. Maybe it's
a problem without a solution, but it can't hurt to think about it
together a bit just in case there are some good ideas out there ☺
1:43 p.m.
Is the pool of code reviewer small? Maybe another unspoken rule of the
'first responder's' to code reviews are the ones who deal with the code in
question, primarily? Does that make sense? You deal with Bodhi so you'd
obviously jump on those CR's whereas someone like Nirik might be a guru
with.. Ansible would jump on those.
That might introduce another layer of complexity though, just spit balling.
On Tue, Jun 27, 2017 at 12:09 PM, Randy Barlow <bowlofeggs(a)fedoraproject.org
wrote:
Hello fellow Fedorans!
As you are probably aware, we have a rule (unspoken? Not sure if it's
formally documented or not) that pull requests in the infrastructure
group should be reviewed by another infra member before they are
merged.
This is a sound policy in my mind, but I've been thinking about a
problem that I've observed that I don't have a solution to. When I
review pull requests that other people write against Bodhi, I am able
to give much higher quality feedback than I am when I review pull
requests on our other projects, simply because I spend a lot of time in
the Bodhi code and am aware of how a lot of it works. For example, I
might suggest "hey, there's a function over in bodhi.server.util that
does what you are doing here - why not use that instead?". However,
when I review code in other projects I am much more limited since I
don't know the code for all of our projects very deeply. Mostly, I can
only make generic comments, or maybe offer some Python hints here and
there.
I don't actually have any proposal of what should change here. I think
reviewing code is good, and I think we should keep doing it. I just
wanted to share an observation I've made when I review code. If anyone
has any ideas on how we could improve this I'd love to hear. Maybe it's
a problem without a solution, but it can't hurt to think about it
together a bit just in case there are some good ideas out there ☺
_______________________________________________
infrastructure mailing list -- infrastructure(a)lists.fedoraproject.org
To unsubscribe send an email to infrastructure-leave@lists.
fedoraproject.org
2:21 p.m.
On Tue, 2017-06-27 at 12:43 -0600, InvalidPath wrote:
Is the pool of code reviewer small? Maybe another unspoken rule of
the 'first responder's' to code reviews are the ones who deal with
the code in question, primarily? Does that make sense? You deal with
Bodhi so you'd obviously jump on those CR's whereas someone like
Nirik might be a guru with.. Ansible would jump on those.
That might introduce another layer of complexity though, just spit
balling.
I do tend to be the main person that reviews Bodhi PRs, and that's
probably good since I know it well. I'm more thinking about the quality
of reviews I can give to other people who know their code the best. For
example, if I review a Pagure PR for Pierre-Yves I feel unable to give
the level of quality I am able to give on Bodhi PRs. And probably
nobody can review that code as well as he can.
This probably stems from the fact that we have many tens of projects,
and not many tens of developers. Thus, there are many projects that
only really have one person that knows it well.
2:45 p.m.
Very true.. expanding the dev base I think is the key you are looking for.
Need to throw more bodies at it :) Could open up possible mentoring
activities but with time being short all around, that might not be viable.
On Tue, Jun 27, 2017 at 1:21 PM, Randy Barlow <bowlofeggs(a)fedoraproject.org>
wrote:
On Tue, 2017-06-27 at 12:43 -0600, InvalidPath wrote:
> Is the pool of code reviewer small? Maybe another unspoken rule of
> the 'first responder's' to code reviews are the ones who deal with
> the code in question, primarily? Does that make sense? You deal with
> Bodhi so you'd obviously jump on those CR's whereas someone like
> Nirik might be a guru with.. Ansible would jump on those.
> That might introduce another layer of complexity though, just spit
> balling.
I do tend to be the main person that reviews Bodhi PRs, and that's
probably good since I know it well. I'm more thinking about the quality
of reviews I can give to other people who know their code the best. For
example, if I review a Pagure PR for Pierre-Yves I feel unable to give
the level of quality I am able to give on Bodhi PRs. And probably
nobody can review that code as well as he can.
This probably stems from the fact that we have many tens of projects,
and not many tens of developers. Thus, there are many projects that
only really have one person that knows it well.
_______________________________________________
infrastructure mailing list -- infrastructure(a)lists.fedoraproject.org
To unsubscribe send an email to infrastructure-leave@lists.
fedoraproject.org
9:31 a.m.
On Tue, Jun 27, 2017 at 02:09:13PM -0400, Randy Barlow wrote:
Hello fellow Fedorans!
As you are probably aware, we have a rule (unspoken? Not sure if it's
formally documented or not) that pull requests in the infrastructure
group should be reviewed by another infra member before they are
merged.
It is mentioned in
https://fedoraproject.org/wiki/Infrastructure/AppBestPractices#New_major_...
but not in its own section so it's not obvious if you're not looking for this.
This is a sound policy in my mind, but I've been thinking about
a
problem that I've observed that I don't have a solution to. When I
review pull requests that other people write against Bodhi, I am able
to give much higher quality feedback than I am when I review pull
requests on our other projects, simply because I spend a lot of time in
the Bodhi code and am aware of how a lot of it works. For example, I
might suggest "hey, there's a function over in bodhi.server.util that
does what you are doing here - why not use that instead?". However,
when I review code in other projects I am much more limited since I
don't know the code for all of our projects very deeply. Mostly, I can
only make generic comments, or maybe offer some Python hints here and
there.
I don't actually have any proposal of what should change here. I think
reviewing code is good, and I think we should keep doing it. I just
wanted to share an observation I've made when I review code. If anyone
has any ideas on how we could improve this I'd love to hear. Maybe it's
a problem without a solution, but it can't hurt to think about it
together a bit just in case there are some good ideas out there ☺
So one of the idea with having people reviewing each others' code is actually to
spread the knowledge about the inner parts of the application.
The idea is that if the application crashes, the more people have seen the code,
or know how someone codes the more chance there is they will be able to
investigate and track quickly the issue (we're of course speaking about the case
when the main developer is away, or just sleeping).
That's one reason why we value a review from a new-comers just as much as a
review from any old-timer.
Basically, we have so many services that we normally have a single point of
contact for a service, so by using a pull-request workflow we started opening
the inner parts of these services to others in the team but we're of course not
able to go into the details of each apps. That means that when I review a PR
coming from the point of contact of the app I most often assume they made the
right choice and I'm more interested in figuring out if the code is
understandable and knowing about the change itself.
We have had time when the PR has allowed restructuring the architecture of the
change itself, but that is surely not the most frequent case.
So to conclude, I agree with you that detailed review is hard and I am not sure
I have a satisfying answer for you right now but still worth trying to think
about it to see what we can improve/change.
Pierre
11:05 a.m.
On Wed, 2017-06-28 at 16:31 +0200, Pierre-Yves Chibon wrote:
So one of the idea with having people reviewing each others' code
is
actually to
spread the knowledge about the inner parts of the application.
+1 - code review certainly has more advantages that just checking each
others' work. Another advantage I've found is that it also helps me
learn new tricks or libraries from time to time.
So to conclude, I agree with you that detailed review is hard and I
am not sure
I have a satisfying answer for you right now but still worth trying
to think
about it to see what we can improve/change.
Thanks for the thoughtful reply. It could be a problem without a viable
solution for us, but if you do think of some ideas I'd love to know
them.
2492
days inactive
2493
days old
infrastructure@lists.fedoraproject.org
5 comments
3 participants
participants (3)
-
InvalidPath -
Pierre-Yves Chibon -
Randy Barlow