Code Owners feature for Code Reviews on Github

[Christian Murphy]

Hi Folks,

A challenge that has been on my mind lately is code review.

Code review is a key part of software development, having more eyes looking over the code helps reduce defects.

However this only works if reviewers have time to review and interest the changes.

In order to find a balance, allowing people to review what they have time to review and interest in, as well as getting all of the code reviewed.

I purpose creating focused groups of reviewers, based on technologies and sub projects.

Ideas on how how it could work:

A CODEOWNERS file would be added to the project.

If you are interested you could edit the CODEOWNERS file to add your username next to what you are interested in reviewing

# Review everything
* @somebody

# Review a specific technology
*.js @person

# Review a specific subproject
docs/* @anotherPerson

For example if a pull request that changes documentation was opened. @somebody and @anotherPerson would be added as potential reviewers automatically.

Would this be something that could be a good fit for uPortal?
Are technologies and subprojects a good way to partition code reviews?

I'd be interested to hear your thoughts.

Best Regards,

Christian Murphy

[Andrew Petro]

+0.

I don't object to people using this CODEOWNERS feature to get to better default targeting for suggested code reviews. It's a text file and some GitHub syntactic sugar. Fair enough, let's try it. Probably the most efficient way to figure out whether it will help uPortal is to just try it.

I nonetheless suspect it's solving problems that either aren't real or that we wouldn't have with better factoring the product into small Git repos for purpose-specific modules.

That /uPortal is a giant monolith with lots and lots of stuff, most of which any given human won't be interested in, is a problem. MyUW doesn't use dynamic skins, but if I watch uPortal I'm exposed to that code. MyUW doesn't store the dynamic skins we don't have in Amazon S3, but again, part of the monolith. Status quo, if you submit a PR that tweaks the S3 storage of the dynamic skins that MyUW doesn't use, I'll be notified of that PR.

 In a different navigation of repo size tradeoffs the dynamic skins feature would be in a tidy little repo for a tidy little optional plugin that realizes skinning in that way, and the S3-backed implementation of that plugin might very well be in own even smaller even tidier repo. And then the repo-level tooling for opting in and out of watching things would be sufficient. There's more value in unpacking the monolith than in trying to mitigate the monolith. Un-pack the monolith and I don't need to use CODEOWNERS to mitigate my view on the noise from the monolith -- I'll simply not watch the small repos in which I can't afford interest.

But if others find this filtering useful, fair enough -- I'd gladly +1 anyone's PR to add themselves to a CODEOWNERS file and to subsequently tweak their CODEOWNERS preferences, and hey maybe even simulating smaller repos with a CODEOWNERS file is a step towards identifying bits to carve out to those smaller repos.

As an aside, I think my own GitHub code reviewing workflows have improved for me with my adoption of Octobox.io. Highly recommend.

-Andrew

On Wednesday, July 12, 2017 at 2:43:28 PM UTC-5, Christian Murphy wrote:

Hi Folks,

A challenge that has been on my mind lately is code review.

Code review is a key part of software development, having more eyes looking over the code helps reduce defects.

However this only works if reviewers have time to review and interest the changes.

In order to find a balance, allowing people to review what they have time to review and interest in, as well as getting all of the code reviewed.

I purpose creating focused groups of reviewers, based on technologies and sub projects.

Ideas on how how it could work:

A CODEOWNERS file would be added to the project.

...

[Misagh Moayyed]

-0

 

1.      Make the change. Ping the PR.

2.      Wait X number of days, gently pinging the PR for feedback.

3.      Merge after X expires.

 

Those who “own” an area may find energy, time and money to review; they know who they are. Putting their name down in a file doesn’t necessarily better either. They are watching the repo. They should be watching the repo. You generally want to watch stuff you “own” right? :) So you merge and if the owners/others find issues with your change, they’re welcomed back to submit yet another PR that fixes what they consider an improvement for a problem. Let them care, on their own.

 

In other words:

 

“You as a contributor may feel inclined to review something, anything, everything, or nothing. Do it as you like, on-demand, organically or in a process-oriented manner of your own design. Do what works for you. Do what you prefer. Provide feedback at all costs, in a timely fashion that works for you. We want to make progress. There is a window. We think it’s reasonable. We should try to fit in, but fret not if you can’t. There is always the next PR; there is always the next release. Keep calm and internalize this”.

 

Also as well intentioned as this may be, CODEOWNERS generally leads to code ownerships and perceptions of such for both the developers and the outside community. Beware.

 

--Misagh

--
You received this message because you are subscribed to the Google Groups "uPortal Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to uportal-dev...@apereo.org.
Visit this group at https://groups.google.com/a/apereo.org/group/uportal-dev/.

[Christian Murphy]

I nonetheless suspect it's solving problems that either aren't real or that we wouldn't have with better factoring the product into small Git repos for purpose-specific modules.

Also as well intentioned as this may be, CODEOWNERS generally leads to code ownerships and perceptions of such for both the developers and the outside community. Beware.

 

Both fair critiques.

How about another approach to improving the pull requests process.

Have uPortal take the position the pull requests will be merged, unless there are changes requested.

This is the approach which uPortal already informally takes with Committers.

We could write that approach down, and consider if or how that could apply all contributions.

Best Regards,

Christian Murphy

[Andrew Petro]

This uportal-dev@ list is way, way, way too quiet.

If anyone has a PR pending that's not getting enough review: talk about it on uportal-dev@. Post to uportal-dev@ saying hey, here's the proposed change, here's why this is important, here's what's going on. Here's the thoughts I had the concerns I had the tradeoffs I navigated. Here's what the shortcomings are, what's yet to be done.

And yes, by all means, invoke Lazy Consensus to force progress as necessary and appropriate.

Under Apache rules, unless Lazy Conensus is invoked, it takes the +1 votes to approve a code change. That'd be votes on-list. I don't think we have the capacity to be anything like this formal -- I'm not saying I object to PRs being merged with approvals signified directly on the PR rather than on list.  Rather, I'm pointing out that the Apache Way is way, way more talking about code changes on list than we're doing.  So hypothesis: there's a lot of daylight to be explored between our status quo practices and the platonic ideal of formal Apache practices, and there are probably some happier-than-status-quo spaces to explore on that continuum.

Invoke Lazy Consensus on uportal-dev@ for any stuck PRs and it'll be impossible for them to stay stuck : either lazy consensus will trigger or someone will engage to prevent it from triggering and viola that's talking about the code. :)

-Andrew

[Drew Wills]

Folks,

I like the way pull requests have historically worked, except that we
should get a little speedier at responding to them.

On 07/13/2017 10:16 AM, Christian Murphy wrote:
> How about another approach to improving the pull requests process.
> Have uPortal take the position the pull requests will be merged,
> unless there are changes requested.

No 2 pull requests are alike. There is no sensible one-size-fits-all
approach for this process.

If you are a contributor, it's important to develop the ability to gauge
the level of controversy inherent in a proposed change.

Some rough examples...
- Fix a code comment? Controversy level 0
- Correct a spelling error in a class/method/variable name?
Controversy level 1-2
- Fix something that wasn't working as documented so that it does?
Controversy level 1-3
- New feature? Controversy level 4+ (lot of variety here)
- Change the markup in any way? Controversy level 6+

The more controversial a change is, the more it requires eyeballs and
consensus. Some things are controversial enough that they should not be
merged without some level of eyeballs and consensus, no matter how long
that takes.

None of us who work on Apereo uPortal do so full time. We tend to get
pulled in other directions for days or occasionally a couple weeks. We
have to be flexible.

> This is the approach which uPortal already informally takes with
> Committers.
> We could write that approach down, and consider if or how that could
> apply all contributions.

Committers are people who have proven themselves to the community and in
whom the community has elevated confidence. We rely on the track record
of committers (and also frequent contributors). We are very excited to
have new people offering contributions; but contributions from new
people really need to have a decent level of scrutiny in every case.

drew

[Christian Murphy]

I like the way pull requests have historically worked, except that we should get a little speedier at responding to them. 

Okay, I'm open to keeping the process relatively the same.

How do we get speedier at responding to pull requests?

None of us who work on Apereo uPortal do so full time.  We tend to get  pulled in other directions for days or occasionally a couple weeks.  We have to be flexible. 

Exactly, the current process seems to struggle with handling this.

If not changing the process, how do we make things better?

Best Regards,

Christian Murphy

[Andrew Petro]

I think we have project participants who skim uportal-dev@ but do not routinely hang out in GitHub looking for notifications. Jim Helwig jumps to mind but I doubt he is alone.

GitHub tooling is great for its tighter interaction with specific code changes, with specific lines of code. Love it. We should keep doing it.

uportal-dev@ is a decent tool for talking about, well, developing uPortal. We should use it more.

Hypothesis: increasing the frequency of traffic on uPortal-dev@ about the Pull Requests will increase the discussion of the concepts in, the changes in, the purposes of the pull requests and will even increase the frequency of attention on the pull requests themselves.

Post here on uportal-dev@, more, about more of the Pull Requests, about more of the why and larger story arc behind the pull requests, about more of the intentions and bigger picture and where we're going with this, and things will improve.

-Andrew

[Christian Murphy]

I think we have project participants who skim uportal-dev@ but do not routinely hang out in GitHub looking for notifications. Jim Helwig jumps to mind but I doubt he is alone.

uportal-dev@ is a decent tool for talking about, well, developing uPortal. We should use it more.
Hypothesis: increasing the frequency of traffic on uPortal-dev@ about the Pull Requests will increase the discussion of the concepts in, the changes in, the purposes of the pull requests and will even increase the frequency of attention on the pull requests themselves.

 

I completely agree with your sentiment that we should increase discussion around PR, and that we should improve outreach to people who do not currently check GitHub.

Playing devils advocate here:

Some PRs are already having to be closed because they get no feedback and bit rot sets in.

Setting the expectation that developers should post all PRs to the list could lead to PRs being closed because "No mailing list post was opened for this, so we never looked at it."

I don't want to be put in a situation where committers have to close PRs, because it never got any feedback (there are still good reasons to close PRs, IMO this is not one of them).

Nor do I want contributors to have to duplicate conversations on GitHub and on the mailing list.

If there is a way to avoid those concerns, I am all for having PRs also posted to the mailing list.

Some potential alternative approaches:

• Encouraging people to participate on GitHub, and leverage GitHub's features to manage notifications.
  
• Having a bot post a summary of PRs to the mailing list each week.
  
• Moving from mailing list to Gitter or Slack, both of which have integrations to show incoming GitHub changes.

Best Regards,

Christian Murphy

[Jim Helwig]

A few thoughts:

While I know I am more than welcome to review PRs, given my distance from the code it would not be a valuable use of my time. I don’t believe I am the target audience here.

I do read the uportal-dev list traffic. I don’t respond to a lot of it, especially if it concerns development details.

I would not like to see all list traffic move to a chat room. Chat seems a bit more conversational and realtime; list posts can be produced/consumed on a slower scale.

That said, at UW we have a chat room dedicated to PRs. I am going to guess that the developers find that handy.

JimH

[Jim Helwig]

Perhaps a good example of this is the recent thread on configuring properties. There is discussion and story arcs on the uportal-dev list. When a PR is created, there could be another plug on the uportal-dev list, “Hey folks, there is a PR that you may be interested in reviewing and commenting on.” Actual discussion of the technical bits of the specific PR would happen in github.

If anyone ever submits a PR that isn’t getting any attention (which we hope does not happen that often), they could poke at the uportal-dev list.

Regardless, more discussion in any format is healthy.

JimH