Code Review Process -- PLEASE READ

[Kevin Klues]

I know we go through this once every couple of months, but yesterday
Ron was again expressing his frustration that our current upstream
code review process has left him unable to effectively do any code
reviews. I share his frustration to a degree.

I find it cumbersome to not be able to easily comment inline on a
commit like you can with browser based tools:

https://github.com/klueska/cputopology/commit/0693e456107f1233d336b8268fb5a29be9ceb67b

I also prefer to look at the diffs in a side by side view with
highlighting on the exact changes that occurred per line:

https://github.com/brho/akaros/commit/9b8d46923913e413e04e34dde080e37ce96dee42

With the current review process, I often find myself opening up the
diffs on github (as above), and flipping back and forth between my
email client and github while doing the review. To do any sort of
inline commenting, I copy a link to the line on the github diff and
then write the comments directly in the email. Doing reviews this way
is essentially the same as doing the review from the terminal, its
just a different way of viewing the diffs.

I'd much prefer to start using the pull request features of github
itself. The biggest win for me being the inline commenting. The
second being the organization / archiving it gives us for associating
feature branches with a discussion. The third being the ease at which
we can bring new developers in. I know our current process is based
off of the linux code review model, but I personally find that model
to be cumbersome as well, and I've actually avoided submitting bugs in
the past because I didn't want to bother with all of the hoops you
have to jump through and was afraid of getting it wrong. I don't want
akaros contributors to have the same hesitations / fears.

It's worth noting that there is a command line tool which exposes many
of github's features to git, including pull requests
(https://github.com/github/hub). Using these tools to submit a pull
request would allow us to (mostly) operate completely from the command
line as we do now. The only real difference being that the code review
would now need to be done on github instead of an email client of some
sort. For those who prefer to always work in the browser, they have
that option too, and can submit their pull request from there. I
personally find this far superior to our current model, and am curious
what others think.

I'd also like to start using the issue tracker as a sort of TODO list
of things we know are broken and need fixing (though it's not
essential to fix them right away). In the past we've used the mailing
list to report this kind of thing, but those tend to get lost in the
sands of time and never resolved. Having a concrete list on the issue
tracker will keep us honest in fixing them eventually. I know I find
myself wanting to knock out small bugs here and there when I'm
frustrated with whatever my bigger project is at the moment. It also
showcases a list of things that need working on to potential
contributors. Using labels we can mark the degree of difficulty in
resolving the issue, giving potential contributors an easy route to
start getting involved.

In general, I think we can make this process work well. However,
there is a big potential downside that is definitely worth noting.
All pull request / issue discussions can only be commented on by
github users if we want them archived on github. We can have github
send the pull requests / issues to the mailing list, but only
developers who have signed up to the mailing list with the same email
they use for github will be able to respond and have their responses
archived on github.

I personally find this as a plus, as I've heard people complaining
that the mailing list is now flooded with uninteresting content. In
my opinion, the mailing list should be for general bigger-idea
discussions, and specific issues / code reviews should be done
elsewhere. As an individual user, you can always sign up to receive
notifications for all pull-requests / issues on github, so individuals
can opt in for this if they like, but separately from the core mailing
list. We can add links to this directly beneath the mailing list link
on the akaros website.

Comments / Questions / Thoughts?

Kevin

[Dan Cross]

I think of our mailing list as really low-traffic; is there something I'm missing? Perhaps a trickle of content that's uninteresting to some, but by no means a flood (there's not enough total volume for that).

I'm a little hesitant about tying ourselves to a particular service vendor, but perhaps we could set up Gerrit somewhere and get more or less the same effect?

[ron minnich]

I absolutely don't want to run a gerrit. I don't see github going away any time soon. 

I think anything we do has to not break the current workflow, but I think we can pull that off.

ron

[barret rhoden]

Hi -

This seems to be the sort of thing where everyone has preferences, and
I'd like something that accommodates them all reasonably. All in all:

- Let's use the issue tracker, in lieu of a better system, for bugs and
TODOs. If someone wants to look into a better system so we can make
a decision, then go right ahead. We can even create an issue for
that.

- I do not want to mandate a github workflow, especially for myself.
If other people want to use it, then great, so long as we can
integrate reasonably into email and a git/shell workflow.

- I think the existing use of the mailing list (detailed code
discussions) is fine for now, given our current volume.

More stuff below:

On 2015-08-27 at 09:52 Kevin Klues <klu...@gmail.com> wrote:
> I know we go through this once every couple of months, but yesterday
> Ron was again expressing his frustration that our current upstream
> code review process has left him unable to effectively do any code
> reviews. I share his frustration to a degree.
>
> I find it cumbersome to not be able to easily comment inline on a
> commit like you can with browser based tools:
>
> https://github.com/klueska/cputopology/commit/0693e456107f1233d336b8268fb5a29be9ceb67b
>
> I also prefer to look at the diffs in a side by side view with
> highlighting on the exact changes that occurred per line:
>
> https://github.com/brho/akaros/commit/9b8d46923913e413e04e34dde080e37ce96dee42

You can comment inline by generating a diff (or git format patch) and
editing the file. As a non-trivial bonus, you can use your actual
editor too. For non-trivial reviews, I'll do this, and have a split
window with the actual code, and things like GTAGS and everything else
I usually do. For the side-by-side review, on the occasion that I use
it, I just use vimdiff.

But I get that those tools can be cumbersome, and just because they
exist, it doesn't mean everyone wants to use them. I'd like everyone
to be happy with their tools.

> With the current review process, I often find myself opening up the
> diffs on github (as above), and flipping back and forth between my
> email client and github while doing the review. To do any sort of
> inline commenting, I copy a link to the line on the github diff and
> then write the comments directly in the email. Doing reviews this way
> is essentially the same as doing the review from the terminal, its
> just a different way of viewing the diffs.
>
> I'd much prefer to start using the pull request features of github
> itself. The biggest win for me being the inline commenting. The
> second being the organization / archiving it gives us for
> associating feature branches with a discussion. The third being the
> ease at which we can bring new developers in. I know our current
> process is based off of the linux code review model, but I personally
> find that model to be cumbersome as well, and I've actually avoided
> submitting bugs in the past because I didn't want to bother with all
> of the hoops you have to jump through and was afraid of getting it
> wrong. I don't want akaros contributors to have the same
> hesitations / fears.

To submit a bug to Akaros you either send us an email or click on the
"Report Bugs" link on the website, which takes you to
https://github.com/brho/akaros/issues. Last I checked, anyone can
email the list; it's just moderated by me due to spammers. For people
contributing code, I've made it very easy. Email a patch or send a
link to a repo (github or otherwise).

> It's worth noting that there is a command line tool which exposes many
> of github's features to git, including pull requests
> (https://github.com/github/hub). Using these tools to submit a pull
> request would allow us to (mostly) operate completely from the command
> line as we do now. The only real difference being that the code review
> would now need to be done on github instead of an email client of some
> sort. For those who prefer to always work in the browser, they have
> that option too, and can submit their pull request from there. I
> personally find this far superior to our current model, and am curious
> what others think.

How about this:

For contributions to any branch I manage (e.g. origin/master), as long
as I get either patches or a git URL (via request pull) in an email,
then I'm happy. I'll respond via email. If people don't want a review
on the list, then email me directly. This should work fine for github
workflow users.

For my own code for origin/master that gets reviewed, I would like a set
of eyes to make sure I didn't screw anything up. If people want to go
to github (or some other tool) to make those comments, then great, let's
try it out and see how it goes.

In the distant future, when we have multiple people contributing code,
when someone manages their own branch, they can do whatever they want
for their patches. Say Ron has a virtio branch and a bunch of people
are working with him. He can use whatever he wants. Likewise, Kevin
and Valmon can do their own thing on the core_alloc/topology branches.

> I'd also like to start using the issue tracker as a sort of TODO list
> of things we know are broken and need fixing (though it's not
> essential to fix them right away). In the past we've used the mailing
> list to report this kind of thing, but those tend to get lost in the
> sands of time and never resolved. Having a concrete list on the issue
> tracker will keep us honest in fixing them eventually.
>
> I know I find myself wanting to knock out small bugs here and there
> when I'm frustrated with whatever my bigger project is at the moment.
> It also showcases a list of things that need working on to potential
> contributors. Using labels we can mark the degree of difficulty in
> resolving the issue, giving potential contributors an easy route to
> start getting involved.

Sounds good. I usually just flag the email in my mailbox, but
consolidating them into one place sounds fine. I'd prefer a bugzilla
or something, but whatever. That's why I have the issue tracker turned
on; it's better than nothing.

Ideally, I'd like it so that when a github issue is created, it pushes
an email to our mailing list. Maybe we can hook that up. If not,
maybe we should look into another tool.

> In general, I think we can make this process work well. However,
> there is a big potential downside that is definitely worth noting.
> All pull request / issue discussions can only be commented on by
> github users if we want them archived on github. We can have github
> send the pull requests / issues to the mailing list, but only
> developers who have signed up to the mailing list with the same email
> they use for github will be able to respond and have their responses
> archived on github.

I'm not a huge fan of the level of integration into github. I mostly
view that as a place to store the repo. I like the distributed nature
of git and that people can put their repos wherever they want.

> I personally find this as a plus, as I've heard people complaining
> that the mailing list is now flooded with uninteresting content.

The Akaros mailing list seems pretty low volume to me. People can
always ignore conversations (e.g. code review) that they don't care
about. If we scale up and it becomes an issue, we can make an
akaros-announce mailing list.

Barret

[Kevin Klues]

I'm trying to think past our current situation where we start
involving other developers. When learning about a project, people
like to be able to browse old code reviews and issues that have been
resolved. They want to quickly see what issues have been filed, the
discussions that have been had surrounding them, and how they have
been resolved. Telling people they have to search a mailing list
archive with inconsistent subject headings for code
reviews/issues/general discussions is not a good way to inspire people
to get involved. Even then, our current mailing list doesn't even
make it easy to search the archives. Only members with passwords can
even see the archives and the sympa UI for viewing/searching the
archives is terrible.

Despite what I said before, I think I agree that the central mailing
list should receive traffic for all types of discussions (development,
code review, issues, etc.), however, I think we also need a way of
easily browsing/searching through each discussion type in a
centralized location on the web. For no other reason than to keep
good historical records and make it easy for newcomers to see what's
been discussed before and what our general issues/code review process
looks like. It's also nice to be able to link back to an old
discussion archived on the web somewhere.

What if we do this. Move our existing mailing list over to google
groups (I've looked up how to pull in the archives of our existing
mailing as part of the migration). We then create tags for
'code-review' and 'issue' so that we can tag discussions with those
labels as appropriate. We can then point people to always post
everything to the mailing list with instructions on how to add the
appropriate tag. We can provide links on our homepage that pre-filter
messages by type. I will take charge of moderating the list to ensure
the appropriate tags are applied.

With this we can disable the issue tracker on github and if people
want to use github pull requests, we can just post a link to the pull
request discussion to the mailing list and tag it with a 'code-review'
label. Moreover, regardless of how the pull reuqests was generated
(mailing list or via github), we can now link to a specific issue via
a url to our archived groups message.

Thoughts?

Kevin

For better or worse, having a well put together README on github
carries alot of weight nowadays. I have a branch ready for review that
adds one (as well as reformats your GETTING_STARTED and Contributing
documents to be markdown compliant), but I've been hesitant to

--
~Kevin

[Kevin Klues]

As an example on my parlib-users group:

All Messages:
https://groups.google.com/forum/#!forum/parlib-users

Issues Only:
https://groups.google.com/forum/#!tags/parlib-users/issue

Code Reviews Only:
https://groups.google.com/forum/#!tags/parlib-users/code-review

--
~Kevin

[ron minnich]

I like the idea of taking one thing at a time, and it seems we're agreed on the github issue tracker, so how about we start with that?

ron

[Dan Cross]

SGTM

[Kevin Klues]

I know I probably won't use the integrated issue tracker without also
using the integrated pull request functionality (they only make sense
to me when used together). I was trying to come up with a compromise
that still kept them unified, but in a more mailing-list centric way
instead of tying them to github. Anyway, I'm done talking about it, I
guess. I just don't understand the pushback. As usual, not much (if
anything) is changing.

--
~Kevin

[Kevin Klues]

> What if we do this. Move our existing mailing list over to google
> groups (I've looked up how to pull in the archives of our existing
> mailing as part of the migration). We then create tags for
> 'code-review' and 'issue' so that we can tag discussions with those
> labels as appropriate. We can then point people to always post
> everything to the mailing list with instructions on how to add the
> appropriate tag. We can provide links on our homepage that pre-filter
> messages by type. I will take charge of moderating the list to ensure
> the appropriate tags are applied.

For lack of a better system we can all agree on, I've started doing
the above to help keep track of issues / code reviews that are
completed / still pending (you may have noticed a few emails coming
through to this effect.)

You can see the tags on the various topics in the google groups web interface:
https://groups.google.com/forum/#!forum/akaros

You can filter by them as (e.g.):
https://groups.google.com/forum/#!tags/akaros/issue
https://groups.google.com/forum/#!tags/akaros/code-review