Enable automated review requests using code owners

98 views
Skip to first unread message

Alex Rodionov

unread,
Jul 7, 2017, 2:43:08 AM7/7/17
to Selenium Developers
Hey folks,

We currently use labels to separate issues and pull requests between maintainers (i.e. I look at “C-rb”, David looks at “C-py”, etc.). It takes some time to go through all of them and properly classify, so I’m thinking about ways to automate this activity.

For pull requests, we can do that using built-in Github code owners functionality. You can read more about this feature in Github blog post. We could have CODEOWNERS file in repository with something like this:

# Python bindings
py/* @AutomatedTester @lucast

# Ruby bindings
rb/* @lucast @p0deje @titusfortner

Once it's done, all new pull requests will be automatically assigned for review to related code owners.

We can then also use this file to analyze new issues with some bot which would try to understand the bindings and assign issues to code owners as well. We can also set up teams in SeleniumHQ organization if writing down maintainers by name is tedious.

What do you think?

Simon Stewart

unread,
Jul 7, 2017, 3:50:09 AM7/7/17
to selenium-developers
This seems like a bit of a solution looking for a problem that's already pretty easy to solve:

git log --pretty=format:'%ce'

It's already possible to ask for reviews on PRs, so I'm not sure what this new feature brings other than extra files in our repos, and a loosening of the belief that anyone with the commit-bit can check in code anywhere they'd like.

What might be a better conversation to have would be to focus on why we have such a backlog of PRs and how we can best address that as a project.

Cheers,

Simon

--
You received this message because you are subscribed to the Google Groups "Selenium Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to selenium-developers+unsub...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/selenium-developers/7e60ba09-fb1e-40e5-b251-60f535a72b75%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

⇜Krishnan Mahadevan⇝

unread,
Jul 7, 2017, 3:58:13 AM7/7/17
to selenium-...@googlegroups.com
Simon,

As one of the people who would like to contribute to Selenium in whatever ways possible, I would also be curious to know what can be done by contributors like me to help get a faster review and merge.

For e.g., here's a PR that I have which is awaiting for review : https://github.com/SeleniumHQ/selenium/pull/4196

There was another PR of mine which I eventually ended up closing since there was no feedback on it for quite some time.

So I would definitely like to know what can I do as a contributor (and also as a person who also would like to earn the committer bit on the selenium codebase someday), do to speed up reviews.


Thanks & Regards
Krishnan Mahadevan

"All the desirable things in life are either illegal, expensive, fattening or in love with someone else!"
My Scribblings @ http://wakened-cognition.blogspot.com/
My Technical Scribbings @ http://rationaleemotions.wordpress.com/

On Fri, Jul 7, 2017 at 1:20 PM, Simon Stewart <simon.m...@gmail.com> wrote:
This seems like a bit of a solution looking for a problem that's already pretty easy to solve:

git log --pretty=format:'%ce'

It's already possible to ask for reviews on PRs, so I'm not sure what this new feature brings other than extra files in our repos, and a loosening of the belief that anyone with the commit-bit can check in code anywhere they'd like.

What might be a better conversation to have would be to focus on why we have such a backlog of PRs and how we can best address that as a project.

Cheers,

Simon
On Fri, Jul 7, 2017 at 7:43 AM, Alex Rodionov <p0d...@gmail.com> wrote:
Hey folks,

We currently use labels to separate issues and pull requests between maintainers (i.e. I look at “C-rb”, David looks at “C-py”, etc.). It takes some time to go through all of them and properly classify, so I’m thinking about ways to automate this activity.

For pull requests, we can do that using built-in Github code owners functionality. You can read more about this feature in Github blog post. We could have CODEOWNERS file in repository with something like this:

# Python bindings
py/* @AutomatedTester @lucast

# Ruby bindings
rb/* @lucast @p0deje @titusfortner

Once it's done, all new pull requests will be automatically assigned for review to related code owners.

We can then also use this file to analyze new issues with some bot which would try to understand the bindings and assign issues to code owners as well. We can also set up teams in SeleniumHQ organization if writing down maintainers by name is tedious.

What do you think?

--
You received this message because you are subscribed to the Google Groups "Selenium Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to selenium-developers+unsubscribe...@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "Selenium Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to selenium-developers+unsub...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/selenium-developers/CAOrAhYFH-5m05SqWQRF%2BKR4%2BMsj8OoVvxJ51GHP0BqW0_Cgu_w%40mail.gmail.com.

Simon Stewart

unread,
Jul 7, 2017, 4:44:13 AM7/7/17
to selenium-developers
Hi Krishnan,

I'll be clear: we do need to grow the base of contributors and find the next generation of leaders within the project. The only way to do that is to accept PRs and help bring people on board.

The way we do things right now is a problem, and I'm not sure how to solve it properly. As you point out, it's causing problems for the project. I'm sorry that you closed the PR --- was it the one that gave Grid a handy RESTful query for finding the state of the Grid?

I know that both Titus and myself are terrible at emails, and that folks like Jim and Luke do what they can, but they also have full time jobs that mean they can't always look at the code. When I'm hacking on Selenium, I try and hang out on the #selenium IRC channel, but relying on someone being available and online at the right time is a terrible way to run a project.

Part of the solution would be for the code developers who are currently less responsive with reviews and PRs to prioritise them a little, but the evidence suggests we've a way to go.

I'd love to hear some ideas and suggestions about what to do. Anything that sends out more emails or relies on us looking at a nice icon on GH may not work that well.

Cheers,

Simon




To view this discussion on the web visit https://groups.google.com/d/msgid/selenium-developers/CAOrAhYFH-5m05SqWQRF%2BKR4%2BMsj8OoVvxJ51GHP0BqW0_Cgu_w%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Selenium Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to selenium-developers+unsub...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/selenium-developers/CANikZLk0AYG9gY40C-vrf2vy2MwBFFi9Mz6L2DB%2B2fu8EaRUBg%40mail.gmail.com.

⇜Krishnan Mahadevan⇝

unread,
Jul 7, 2017, 5:08:55 AM7/7/17
to selenium-...@googlegroups.com
Simon,

Yes it was the RestFul APIs for the Grid PR that I had to close of. 

As people who either casually contribute or as people who are first time contributors, getting feedback in a timely manner on a PR helps us fix review comments faster. I say this because since we aren't that familiar with the codebase, if the feedback comes a bit later, it takes time for me at-least to get back to the old rhythm and thought process and thus increases the turn around time. Sometimes I even get lost in the process. But am sure that's only me :) 

More than sending emails or hoping that people look at their GH notifications, I currently can't think of anything else.

But assuming we are comfortable with emails and which we as Selenium Dev promise ourselves to take a look and act upon whenever we have the time to spare, I was hoping we could consider something like below :
  1. Create an e-mail distribution list of contributors on a per component/technology stack basis and also perhaps a GitHub team for the same.
  2. Publish this information (e-mail distribution list and the corresponding GitHub Team for a given component/tech stack) on our Wiki page : https://github.com/SeleniumHQ/selenium/wiki
  3. Every-time a contributor raises a Pull Request have them either ping the corresponding GitHub team (or) send an email to the relevant distribution list with a link to the Pull Request. If required we could even enhance our Pull Request template to capture this details
This would at-least help consolidate the list of Pull Requests on a per component and per technology stack and make it available for the current Selenium dev to see.

But I don't know how to make the current set of dev who have commit access, to act on it because as you said, everyone has a day job and they are trying to contribute their free time for this project without expecting any returns and everyone of us including me value that time. So its unfair to demand.

On a side note : Thanks for the feedback on my PR :) I have addressed your comments. If you are fine with the fix, it would be great if you can help with the merge as well.


Thanks & Regards
Krishnan Mahadevan

"All the desirable things in life are either illegal, expensive, fattening or in love with someone else!"
My Scribblings @ http://wakened-cognition.blogspot.com/
My Technical Scribbings @ http://rationaleemotions.wordpress.com/

To view this discussion on the web visit https://groups.google.com/d/msgid/selenium-developers/CANikZLk0AYG9gY40C-vrf2vy2MwBFFi9Mz6L2DB%2B2fu8EaRUBg%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Selenium Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to selenium-developers+unsub...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/selenium-developers/CAOrAhYGGSt8awuMX3Ok%2Bu8UG0U21vw6aKi4YP3sonuw8W0Mjgg%40mail.gmail.com.

Simon Stewart

unread,
Jul 7, 2017, 8:05:41 AM7/7/17
to selenium-developers
Hi,

We have labels that can be applied to each PR. I _think_ anyone can apply a label to a PR when its being created, but it's entirely possible that I'm mistaken. When I do remember to look at the incoming PRs, those are the first thing I filter on.

I note that the GH UI will also suggest reviewers, but (again) perhaps this is because I'm an committer?

We should probably add a mapping between email and GH user ID somewhere discoverable; good idea.

The information about the best person to ask for a review is already in the project's git history:

git shortlog -cesn --since=6.months your/directory/here

I suspect I'm a bit of an outlier in how I use and abuse these tools. If anyone else with GitHub experience wants to join the conversation, please do so :)

The PR is now merged. Congrats! 

Cheers,

Simon

To view this discussion on the web visit https://groups.google.com/d/msgid/selenium-developers/CAOrAhYGGSt8awuMX3Ok%2Bu8UG0U21vw6aKi4YP3sonuw8W0Mjgg%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Selenium Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to selenium-developers+unsub...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/selenium-developers/CANikZL%3DLozvS-55R82Yu9y3UnrPUDp3ArKeic73Y5%2B9-sr56-w%40mail.gmail.com.

Alex Rodionov

unread,
Jul 7, 2017, 9:49:46 AM7/7/17
to Selenium Developers
Hi,

Let me comment on some of these things:

We have labels that can be applied to each PR. I _think_ anyone can apply a label to a PR when its being created, but it's entirely possible that I'm mistaken. 

Only committers can add/remove labels, nobody else can. This is why I (and others like David, Lucas, Luke) spend some time every day to go through each issue/pull request and add correct labels.

I note that the GH UI will also suggest reviewers, but (again) perhaps this is because I'm an committer?

GitHub suggests people based on git activity, but again only committers can assign reviewers. 

> When I do remember to look at the incoming PRs, those are the first thing I filter on.

What if instead of filtering by labels you would filter by "Assigned to me"?

Overall, you guys pretty much suggest the same thing I initially proposed :) We can create teams like @seleniumhq/java, @seleniumhq/rb and use GitHub code owners feature to automatically assign members of the matching team as reviewers for PR. That will:

* send email notifications (and maybe in other tools that integrate with GitHub API) to matched team members
* allow any committer to simply open PRs assigned to them and review
* allow us to skip classifying PRs with labels (in most cases)

On a side note, I'm not fond of depending on GitHub, but: 

* we already depend on it (see .github/ directory)
* code owners feature is super easy to set up

The other approach to do the same would require building a standalone bot listening to all incoming PRs, analyzing them and assigning proper labels. I would imagine such bot to be something like Rails team has and I'd like to implement it (at least because we still have to manually classify issues), but it would take a while to be done as I also have a full-time job :)
To unsubscribe from this group and stop receiving emails from it, send an email to selenium-developers+unsub...@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "Selenium Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to selenium-developers+unsub...@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "Selenium Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to selenium-developers+unsub...@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "Selenium Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to selenium-developers+unsub...@googlegroups.com.

Simon Stewart

unread,
Jul 7, 2017, 1:18:12 PM7/7/17
to selenium-developers
On Fri, Jul 7, 2017 at 2:49 PM, Alex Rodionov <p0d...@gmail.com> wrote:
Hi,

Let me comment on some of these things:

We have labels that can be applied to each PR. I _think_ anyone can apply a label to a PR when its being created, but it's entirely possible that I'm mistaken. 

Only committers can add/remove labels, nobody else can. This is why I (and others like David, Lucas, Luke) spend some time every day to go through each issue/pull request and add correct labels.

I knew you were awesome :) 

Doesn't look like the GH folks are keen on allowing users to update labels: https://github.com/isaacs/github/issues/148 That's a real pity. Would have made life a lot easier.
 
I note that the GH UI will also suggest reviewers, but (again) perhaps this is because I'm an committer?

GitHub suggests people based on git activity, but again only committers can assign reviewers. 

*sigh* 
 
> When I do remember to look at the incoming PRs, those are the first thing I filter on.

What if instead of filtering by labels you would filter by "Assigned to me"?

I'm deathly afraid of what that might look like :) *Goes and looks* Oh! That could be a lot worse.
 
Overall, you guys pretty much suggest the same thing I initially proposed :) We can create teams like @seleniumhq/java, @seleniumhq/rb and use GitHub code owners feature to automatically assign members of the matching team as reviewers for PR. That will:

* send email notifications (and maybe in other tools that integrate with GitHub API) to matched team members
* allow any committer to simply open PRs assigned to them and review
* allow us to skip classifying PRs with labels (in most cases)

The downside is "owners" files. Right now, there are people who are most knowledgable about particular parts of the tree, and (of course) it's polite to ask permission before going and changing everything. Having an owner changes the mental model from "polite" to "mandated", which sucks.

Also, IME assigning a review to more than one person is a recipe for it to never be done: everyone believes that the other reviewers will do it for them. Assigning a review to someone who's too busy or can't get to the review also doesn't help, since the PR also sits there uselessly. Which suggests that auto-assigning reviews may not work.

Maybe my experience is different from other peoples? 

Perhaps if selbot would DM people on the IRC channel with a selection of PRs to review that might work?
 
On a side note, I'm not fond of depending on GitHub, but: 

* we already depend on it (see .github/ directory)
* code owners feature is super easy to set up

"Easy to set up" and "useful feature" are orthogonal to each other :)
 
I'd be fine experimenting with the owners feature for a bit, but I'm just not sure it's something that fits with an Open Source project. It seems like something that's far more useful in a context where people are paid to care and can be held accountable by their managers.

We do need to improve things, but I'd like us to move slowly in the right direction, rather than quickly in the direction of anything that looks shiny. "Owners" looks like a square peg that we're tempted to try and fit into a round hole.

The other approach to do the same would require building a standalone bot listening to all incoming PRs, analyzing them and assigning proper labels. I would imagine such bot to be something like Rails team has and I'd like to implement it (at least because we still have to manually classify issues), but it would take a while to be done as I also have a full-time job :)

Side projects are cool :) 

Cheers,

Simon
 
To unsubscribe from this group and stop receiving emails from it, send an email to selenium-developers+unsubscribe...@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "Selenium Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to selenium-developers+unsubscribe...@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "Selenium Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to selenium-developers+unsubscribe...@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "Selenium Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to selenium-developers+unsubscribe...@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "Selenium Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to selenium-developers+unsubscribe...@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "Selenium Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to selenium-developers+unsub...@googlegroups.com.

p0deje

unread,
Jul 7, 2017, 10:12:19 PM7/7/17
to selenium-developers
I haven’t really thought about mental change when talking about “owners”. For me it’s more like a reference of people who has better expertise of different components. The codebase is still owned by everyone of course, but some people know some parts better.

I don’t really want to change to “mandate”, so if there is a chance of this happening, let’s not use code owners.

However, I think we’ve started a good discussion on how we can improve the current situation with issues (510 open) and pull requests (151 open). If we can continue conversation in this thread and figure out the next steps to improve response/review time, that would be good.

I propose to formulate the steps required for review that can be automated and then try to implement them in bot. Though I’m not sure what are these steps.

To unsubscribe from this group and stop receiving emails from it, send an email to selenium-develo...@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "Selenium Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to selenium-develo...@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "Selenium Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to selenium-develo...@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "Selenium Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to selenium-develo...@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "Selenium Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to selenium-develo...@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "Selenium Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to selenium-develo...@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "Selenium Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to selenium-develo...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/selenium-developers/CAOrAhYHmwk-WNQ-rGW7C5rySCjAUexbLvHu06egD3PGsck59FA%40mail.gmail.com.

Diego Fernando Molina

unread,
Jul 10, 2017, 10:46:35 AM7/10/17
to Selenium Developers
Hi,

Being fairly new to this group, just let me add my two cents.

I started to help with the docker-selenium repo, which has smaller numbers and therefore it could be easier to manage. The approach I have been using is:
  • Going through all open issues and in many old ones, I asked for an update. In some cases it was already fixed and in other cases we didn't get an answer. At the end, with Dan's help, we reduced the # of issues from ~110 to ~60.
    • Yes, it took time to read all the issues and understand them, but it also helped a lot to clean up and to understand the project itself.
  • ~70% of the issues that remained open, are worthy to investigate and check what the problem is -> this is the next step. (The remaining 30% are waiting for OP feedback, they could be potentially closed.)
  • About the open PRs (around ~15), apply the same procedure as above. 
That was for the "now", but it is clear that this is not 100% sustainable. So some next steps could be:

  • Improve the tests for PRs and releases -> Dan already started and I am jumping in to help there. In the future, we should not consider PRs that don't pass the tests (exceptions are always there of course).
  • Have a bot to reply to issues and PRs, which should suggest reviewers as well. (Needs to be discussed with Dan). At the end I would like to have something like the k8s guys have, check this example.

What I am trying to say is that we could do:
  • A general clean up (I volunteer to help after my Summer vacation)
  • Improve the tests for PRs. Highly important, right now I see that in all PRs the build failed. So committers could review PRs that have tests passing (with counted exceptions of course) and a successful build.
  • Adding some bot logic to assign PRs. 

Do you guys think that this approach could work?

Cheers,
To unsubscribe from this group and stop receiving emails from it, send an email to selenium-developers+unsub...@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "Selenium Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to selenium-developers+unsub...@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "Selenium Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to selenium-developers+unsub...@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "Selenium Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to selenium-developers+unsub...@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "Selenium Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to selenium-developers+unsub...@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "Selenium Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to selenium-developers+unsub...@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "Selenium Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to selenium-developers+unsub...@googlegroups.com.
Reply all
Reply to author
Forward
0 new messages