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_feature 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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ infrastructure mailing list -- infrastructure@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to infrastructure-leave@xxxxxxxxxxxxxxxxxxxxxxx
