Patch review for OpenLMI

[Jan Safranek]

Hi,

we've talked on Friday about starting code review for all OpenLMI
patches. Fedora 19 alpha freeze is here and it could be good milestone
to start with reviews (or at least as good as any other milestone).

I would prefer as light review process as possible, we're still in
initial development where we code pretty quickly and I personally don't
want to get slowed down by reviewing each patch character-by-character.

@sgallahg, you mentioned some review tool, what was it and
where/when/how can we use it?

Jan

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Mon 11 Mar 2013 10:35:51 AM EDT, Jan Safranek wrote:
>  Hi,
>  
>  we've talked on Friday about starting code review for all OpenLMI 
>  patches. Fedora 19 alpha freeze is here and it could be good
>  milestone to start with reviews (or at least as good as any other
>  milestone).
>  
>  I would prefer as light review process as possible, we're still in 
>  initial development where we code pretty quickly and I personally
>  don't want to get slowed down by reviewing each patch
>  character-by-character.
>   
I agree. While we're still in heavy development, we probably want to
limit our patch reviews to be ungated. (I.e. they shouldn't block
forever waiting for a review).

I'm inclined to suggest that we take the following approach:
Patches are sent to the openlmi-devel list as attachments.
If another developer reviews it and replies with "Ack", it can be
pushed immediately.
If another developer reviews it and replies with "Nack", it must be fixed.
If it's not reviewed within two business days, push it to master.


>  @sgallahg, you mentioned some review tool, what was it and 
>  where/when/how can we use it? 

The review tool I was talking about was Review Board[1], and I'm
working on getting an instance of it hosted. I was planning to launch
it in an OpenShift instance today, but I'm having some issues with
their hosting (it looks like all of the static data like the CSS isn't
properly reachable, so it looks terrible).

I will sort this out and provide an update as soon as possible.

[1] http://www.reviewboard.org
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.13 (GNU/Linux)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
>
> iEYEARECAAYFAlE+JrsACgkQeiVVYja6o6MAKwCfSlGu8OQWsh0bZjWLiCR6duyG
> z4QAoKiTA//34cPaO8pKG6pBXf/1aIHs
> =Pw4K
> -----END PGP SIGNATURE-----

[Roman Rakus]

On 03/11/2013 07:47 PM, Stephen Gallagher wrote:
>  Patches are sent to the openlmi-devel list as attachments.
Hello. I will rather use git-send-email tool. It means, patches are not 
sent as attachments.

RR

[Tomáš Smetana]

On Mon, 11 Mar 2013 21:03:05 +0100
Roman Rakus <rrakus(a)redhat.com&gt; wrote:

>  On 03/11/2013 07:47 PM, Stephen Gallagher wrote:
>  > Patches are sent to the openlmi-devel list as attachments.
>  Hello. I will rather use git-send-email tool. It means, patches are not 
>  sent as attachments. 
If we're to send patches to the list I'd also vote for the inline text: easier
to answer with comments right in the reviewed code.

Thanks and regards,
-- 
Tomáš Smetana
Platform Engineering, Red Hat

[Jan Safranek]

On 03/11/2013 07:47 PM, Stephen Gallagher wrote:
>  I'm inclined to suggest that we take the following approach:
>  Patches are sent to the openlmi-devel list as attachments.
>  If another developer reviews it and replies with "Ack", it can be
>  pushed immediately. 
I'd rather wait for the review tool to be online, it may have different
workflow than sending patches to -devel and IMHO we should avoid
changing the workflow just in few days.

> > @sgallahg, you mentioned some review tool, what was it and 
> > where/when/how can we use it?
>  
>  
>  The review tool I was talking about was Review Board[1], and I'm
>  working on getting an instance of it hosted. I was planning to launch
>  it in an OpenShift instance today, but I'm having some issues with
>  their hosting (it looks like all of the static data like the CSS isn't
>  properly reachable, so it looks terrible).
>  
>  I will sort this out and provide an update as soon as possible. 
I am not sure I get it right, it seems the patches must be manually
uploaded using their post-review cmdline tool (ok) and when the patches
are accepted, they must be manually checked in the upstream git. I.e. I
still need to merge my topic branch with master and push it?

I know lot of projects use Gerrit, I have no personal experience with
it, but it seems more git-friendly. Still, does anyone have actual
experience with Review Board or Gerrit?

Jan

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Tue 12 Mar 2013 04:11:03 AM EDT, Jan Safranek wrote:
>  On 03/11/2013 07:47 PM, Stephen Gallagher wrote:
> > I'm inclined to suggest that we take the following approach: 
> > Patches are sent to the openlmi-devel list as attachments. If 
> > another developer reviews it and replies with "Ack", it can be 
> > pushed immediately.
>  
>  I'd rather wait for the review tool to be online, it may have 
>  different workflow than sending patches to -devel and IMHO we 
>  should avoid changing the workflow just in few days.
>   
I agree. I'll try to get back to focusing on this as soon as the
Fedora Alpha Freeze chaos is over.

> >> @sgallahg, you mentioned some review tool, what was it and 
> >> where/when/how can we use it?
> > 
> > 
> > The review tool I was talking about was Review Board[1], and I'm
> >  working on getting an instance of it hosted. I was planning to 
> > launch it in an OpenShift instance today, but I'm having some 
> > issues with their hosting (it looks like all of the static data 
> > like the CSS isn't properly reachable, so it looks terrible).
> > 
> > I will sort this out and provide an update as soon as possible.
>  
>  I am not sure I get it right, it seems the patches must be manually
>  uploaded using their post-review cmdline tool (ok) and when the
>  patches are accepted, they must be manually checked in the upstream
>  git. I.e. I still need to merge my topic branch with master and
>  push it?
>  
>  I know lot of projects use Gerrit, I have no personal experience 
>  with it, but it seems more git-friendly. Still, does anyone have 
>  actual experience with Review Board or Gerrit?
>   
First, a bit of disclosure: I've been an intermittent developer on the
Review Board project for about three years. I have a lot of
familiarity with it and I'm collaborating with upstream on enhancing
its git workflow capabilities. That being said, its current interface
is not designed for commit/merge workflow the same way that Gerrit or
Github are, but that's intentional. Handling merges is a complicated
bit of business and Review Board is focused on the review workflow
rather than the commit workflow.

I've used Review Board, Gerrit and Github on multiple occasions, and
without any doubt I will say that Review Board has the most
comprehensive, powerful and easy to use *review* interface. Gerrit has
the best merge interface (and a terrible review interface) and Github
is kind of mediocre on both points.

There is work in Review Board upstream right now targeted at their 2.0
release to support more of the git workflow pieces like pull requests
and merges. They *are* coming, and when it lands, I fully expect
Review Board at that point to be a clear advantage over Gerrit (along
with fully integrated with Github; the work going into the
patch-series and merge handling is planning to be able to use Github's
public API)


So yes, if we opt for Review Board, we're still going to be doing
manual merges to the upstream repository. I don't necessarily think
that's a bad thing, since it requires a final manual step to make sure
that nothing obviously breaks (although this can be minimized with
continuous integration testing).


> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.13 (GNU/Linux)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
>
> iEYEARECAAYFAlE/EiYACgkQeiVVYja6o6NK7wCePgXSaHLsk9L3FeBD5NEUzWic
> 2F4AoIr2M77fxyUoyy/HFEepOF8Miwmh
> =5/Ud
> -----END PGP SIGNATURE-----

[Jan Safranek]

On 03/12/2013 12:31 PM, Stephen Gallagher wrote:
>  I've used Review Board, Gerrit and Github on multiple occasions,
> and
>  without any doubt I will say that Review Board has the most
>  comprehensive, powerful and easy to use *review* interface. Gerrit has
>  the best merge interface (and a terrible review interface) and Github
>  is kind of mediocre on both points. 
<snip>

>  So yes, if we opt for Review Board, we're still going to be
> doing
>  manual merges to the upstream repository. I don't necessarily think
>  that's a bad thing, since it requires a final manual step to make sure
>  that nothing obviously breaks (although this can be minimized with
>  continuous integration testing). 
I have direct experience only with GitHub and hasn't been pleasant so
far. I trust your experience and I'd vote for Review Board. Only time
shows if it is good tool.

BTW, reading mediawiki's Gerrit_evaluation, I am quite surprised it does
not allow review of patch series. I usually have multiple patches in one
branch, I polish them and then I send whole patchset for review, does
Review Board support this?

Jan

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Tue 12 Mar 2013 08:31:15 AM EDT, Jan Safranek wrote:
>  On 03/12/2013 12:31 PM, Stephen Gallagher wrote:
> > I've used Review Board, Gerrit and Github on multiple occasions,
> > and without any doubt I will say that Review Board has the most 
> > comprehensive, powerful and easy to use *review* interface.
> > Gerrit has the best merge interface (and a terrible review
> > interface) and Github is kind of mediocre on both points.
>  
>  <snip>
>  
> > So yes, if we opt for Review Board, we're still going to be
> > doing manual merges to the upstream repository. I don't
> > necessarily think that's a bad thing, since it requires a final
> > manual step to make sure that nothing obviously breaks (although
> > this can be minimized with continuous integration testing).
>  
>  I have direct experience only with GitHub and hasn't been pleasant
>  so far. I trust your experience and I'd vote for Review Board. Only
>  time shows if it is good tool.
>  
>  BTW, reading mediawiki's Gerrit_evaluation, I am quite surprised it
>  does not allow review of patch series. I usually have multiple
>  patches in one branch, I polish them and then I send whole patchset
>  for review, does Review Board support this?
>   

No, not yet. It's under active development as part of Review Board 2.0
(hopefully coming out in late 2013). I'm on the planning team for that
feature (mostly because I put myself there and won't shut up about it :) )

Some background on the topic:
https://groups.google.com/d/msg/reviewboard-dev/7ungX3jLoXk/PLN5Nx3KwrIJ
(It's fallen off in discussion in the last couple months, mostly due
to the release of 1.7 and other distractions. I need to bring that
thread back to life). I've also had some private discussions with the
upstream lead on the topic that should probably be summarized there...
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.13 (GNU/Linux)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
>
> iEYEARECAAYFAlE/ISIACgkQeiVVYja6o6NzBwCgoz1ezr+3GP5x4aQPBqibT3EU
> a74AoJ9LWIN35RWEuXvPu/B5KArBD4TJ
> =zdmm
> -----END PGP SIGNATURE-----

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/12/2013 07:31 AM, Stephen Gallagher wrote:
>  On Tue 12 Mar 2013 04:11:03 AM EDT, Jan Safranek wrote:
> > On 03/11/2013 07:47 PM, Stephen Gallagher wrote:
> >> I'm inclined to suggest that we take the following approach: 
> >> Patches are sent to the openlmi-devel list as attachments. If 
> >> another developer reviews it and replies with "Ack", it can be
> >>  pushed immediately.
>  
> > I'd rather wait for the review tool to be online, it may have 
> > different workflow than sending patches to -devel and IMHO we 
> > should avoid changing the workflow just in few days.
>  
>  
>  I agree. I'll try to get back to focusing on this as soon as the 
>  Fedora Alpha Freeze chaos is over. 

As promised, I have made a Review Board instance available at
https://reviewboard-openlmi.rhcloud.com/dashboard/

I haven't tied it to FAS yet (that's going to wait until we have
OpenID support added to Review Board in the next few months; I have an
intern working on adding that feature). I enabled self-registration on
the server, so anyone can create a (nonprivileged) account. There's a
bug I haven't figured out yet where it's hanging without response when
you correctly register an account. You can just click "Log In" in the
upper-right at this point and enter your username and password though.


There are two ways that you can submit a patch for review. The most
automated way is to install the RBTools package on Fedora or EPEL and
then use the 'post-review' binary. This will submit a review based on
changes compared to the most recent git branch point (i.e. all your
changes from master). If you pass the --guess-fields (or -g) option,
it will generate the web review description from the patch commit
descriptions. see 'post-review --help' or [1] for more ideas.

Otherwise, you can upload a diff directly to the web interface from
the "New Review Request" tab.


Some tips for reviews: you can drag-select a section of the diff on
the web interface to select it for comment. You also need to make sure
to click both "save" (which saves your current progress on this
review) and "publish" (which finalizes the review and makes it public
and sends email[2] to the subscribed watchers)

I'd like to suggest we give this a whirl and see how we feel about it.


[1] http://www.reviewboard.org/docs/manual/dev/users/tools/post-review/
[2] I haven't tested the email sending yet; it may be a little tricky
to get it to deliver properly because the OpenShift instance is hosted
in the Amazon cloud, which many email providers are blacklisting.
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.13 (GNU/Linux)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
>
> iEYEARECAAYFAlFCNK0ACgkQeiVVYja6o6Oi/QCgrCzxnwG5ZmjqBG56P4rfXoGe
> xFUAoI/gRmZgVI378DVjhj3yXRuSyvRJ
> =uZD1
> -----END PGP SIGNATURE-----

[Jan Safranek]

On 03/14/2013 09:35 PM, Stephen Gallagher wrote:
>  There are two ways that you can submit a patch for review. The most
>  automated way is to install the RBTools package on Fedora or EPEL and
>  then use the 'post-review' binary.  
I tried and I failed:

$ post-review -g -o --server=https://reviewboard-openlmi.rhcloud.com
--username=jsafrane

There was an error creating this review request.

The repository path
"git+ssh://[e-mail]/git/openlmi-storage.git" is not
in the
list of known repositories on the server.


</cite>

I see some openlmi-storage in reviewboard-openlmi.rhcloud.com/r/new/,
but it seems to expect different repo path.

Jan

[Jan Safranek]

On 03/15/2013 05:40 PM, Jan Safranek wrote:
>  On 03/14/2013 09:35 PM, Stephen Gallagher wrote:
> > There are two ways that you can submit a patch for review. The most
> > automated way is to install the RBTools package on Fedora or EPEL and
> > then use the 'post-review' binary. 
>  
>  I tried and I failed:
>  
>  $ post-review -g -o --server=https://reviewboard-openlmi.rhcloud.com
>  --username=jsafrane
>  
>  There was an error creating this review request.
>  
>  The repository path
>  "git+ssh://[e-mail]/git/openlmi-storage.git" is not
>  in the
>  list of known repositories on the server.
>  
>  
>  </cite>
>  
>  I see some openlmi-storage in reviewboard-openlmi.rhcloud.com/r/new/,
>  but it seems to expect different repo path. 
Weird, when I type wrong password, I get the same message, so it may be
not related to the git repo URL.

Jan

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/15/2013 12:40 PM, Jan Safranek wrote:
>  On 03/14/2013 09:35 PM, Stephen Gallagher wrote:
> > There are two ways that you can submit a patch for review. The
> > most automated way is to install the RBTools package on Fedora or
> > EPEL and then use the 'post-review' binary.
>  
>  I tried and I failed:
>  
>  $ post-review -g -o
>  --server=https://reviewboard-openlmi.rhcloud.com 
>  --username=jsafrane
>  
>  There was an error creating this review request.
>  
>  The repository path 
>  "git+ssh://[e-mail]/git/openlmi-storage.git"
>  is not in the list of known repositories on the server.
>  
>  
>  </cite>
>  
>  I see some openlmi-storage in
>  reviewboard-openlmi.rhcloud.com/r/new/, but it seems to expect
>  different repo path.
>  
>  Jan
>   
Yeah, what you want to do is add a .reviewboardrc file to the root of
the repo checkout containing:

REVIEWBOARD_URL = 'https://reviewboard-openlmi.rhcloud.com'
REPOSITORY = 'openlmi-storage'


If you had cloned the repository from the anonymous URL
(git://git.fedorahosted.org/git/openlmi-storage.git), post-review
would have been able to automatically figure out to which repo to
submit the patch. But since your checkout was using the committer URL,
you need to just tell Review Board explicitly which one to use for
this checkout.

If we decide to stick with Review Board in the future, we'll want to
just commit that .reviewboardrc to the repository so that it will Just
Work for anyone in the future.

Also, as you probably can figure out, you need to change the
REPOSITORY= line appropriately for the other git repos as well.
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.13 (GNU/Linux)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
>
> iEYEARECAAYFAlFDVfQACgkQeiVVYja6o6OtQwCcCoGc6fiCTHjF1nPnGndPJLG5
> r8kAoI95e+viXboW4oK+3eyec5kHGnBx
> =FRFU
> -----END PGP SIGNATURE-----

[Radek Novacek]

On Mon 11 of Mar 2013 15:35:51 Jan Safranek wrote:
>  Hi,
>  
>  we've talked on Friday about starting code review for all OpenLMI
>  patches. Fedora 19 alpha freeze is here and it could be good milestone
>  to start with reviews (or at least as good as any other milestone).
>  
>  I would prefer as light review process as possible, we're still in
>  initial development where we code pretty quickly and I personally don't
>  want to get slowed down by reviewing each patch character-by-character.
>  
>  @sgallahg, you mentioned some review tool, what was it and
>  where/when/how can we use it?
>  
>  Jan 
I personally prefer some web interface for reviewing patches. Advantages for 
using some tool from my point of view (might not be applicable for all review 
tools):
* one click for apply the patch
* inline comments (discussion is easier to read than in the mail thread, 
especially with more parallel discussions)
* showing more context of the patch hunk is easier (just click vs applying to 
local checkout)
* possibility to send notification about review to mailing list too

On the other hand, advantages for using mails:
* easy to submit for review via git-send-email
* acks are visible in mailing list (can review tools be configured to do it?)

Feel free to add more pros and cons for both approaches. 

Radek Novacek

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/12/2013 05:47 AM, Radek Novacek wrote:
>  On Mon 11 of Mar 2013 15:35:51 Jan Safranek wrote:
> > Hi,
> > 
> > we've talked on Friday about starting code review for all
> > OpenLMI patches. Fedora 19 alpha freeze is here and it could be
> > good milestone to start with reviews (or at least as good as any
> > other milestone).
> > 
> > I would prefer as light review process as possible, we're still
> > in initial development where we code pretty quickly and I
> > personally don't want to get slowed down by reviewing each patch
> > character-by-character.
> > 
> > @sgallahg, you mentioned some review tool, what was it and 
> > where/when/how can we use it?
> > 
> > Jan
>  
>  I personally prefer some web interface for reviewing patches.
>  Advantages for using some tool from my point of view (might not be
>  applicable for all review tools): * one click for apply the patch 
I don't know of any review tools that meet this requirement. In the
case of git, some tools can allow you to register a (virtual) remote
repo branch that includes the applied patch, but if you want to merge
it to your own in-progress branches, that's going to be a manual step
no matter what.

In the case of Review Board, one of it's current major bugs is that
the downloaded patch comes back to you as a pure diff (without the
extended git header), which means that it's trickier to apply locally
than we would like. In this case, there's an argument to be made for
Gerrit, which creates the virtual branches like I mentioned above.

>  * inline comments (discussion is easier to read than in the mail
>  thread, especially with more parallel discussions) 
Review Board is simply the best at this. Gerrit is pretty poor at it
(capable, but not as clear and easy to follow). Github only allows
general comments on a pull request; they do not allow inline comments,
last I checked.

>  * showing more context of the patch hunk is easier (just click vs
>  applying to local checkout) 
Review Board, Gerrit and Github can do this easily.

>  * possibility to send notification about review to mailing list
>  too
>   
All three have this capability, with different degrees of information
about what was updated.

>  On the other hand, advantages for using mails: * easy to submit for
>  review via git-send-email 
Gerrit wins here, as you can do a 'git push' to the Gerrit server and
it can be configured to also send the 'git-send-email' messages to a
mailing list.

>  * acks are visible in mailing list (can review tools be configured
>  to do it?)
>   
All three review tools discussed above can do this (in different ways).

Review Board: There is a "Ship It!" flag that can be set on the
review, which is functionally an Ack.

Gerrit: Reviews are configured to require a certain number of acks
(possibly from specific groups of users). When this is met, the
gatekeepers get a "merge" button enabled. All reviews are sent to the
list, including whether it was reviewed as "Fully acked", "Acked but I
want someone else to look too" or "Nacked".

Github: All reviews are comments on the pull request and all reviews
are mailed out. So just adding a comment "Ack" meets this requirement.


>  Feel free to add more pros and cons for both approaches.
>  
>  Radek Novacek _______________________________________________ 
>  openlmi-devel mailing list openlmi-devel(a)lists.fedorahosted.org 
>  https://lists.fedorahosted.org/mailman/listinfo/openlmi-devel
>   
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.13 (GNU/Linux)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
>
> iEYEARECAAYFAlE/FHoACgkQeiVVYja6o6Mp0QCfWp9tZluRXTI5kArrtAEaGrJv
> f0cAoId8DwwmpkIg56M6suPHJNO3N8LF
> =cyxp
> -----END PGP SIGNATURE-----

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/12/2013 05:47 AM, Radek Novacek wrote:
>  Feel free to add more pros and cons for both approaches.
>   

Actually, the Mediawiki project put together a REALLY comprehensive
analysis of Gerrit vs alternatives here:
http://www.mediawiki.org/wiki/Git/Gerrit_evaluation

Mostly, I'd recommend reading the two sections
 * "The case for Gerrit"
 * "The case against Gerrit"


They also cover a lot of detail about alternatives such as Github,
Gitlab and Phabricator. They don't touch on Review Board very much,
mostly because they established that one of their must-haves was the
"It must support automatic merge on approve", which Review Board does
not meet.
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.13 (GNU/Linux)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
>
> iEYEARECAAYFAlE/FgMACgkQeiVVYja6o6MV/gCfbTwsvwcHFCLapTQw9LdYc301
> AbAAoLEWkpkJhfuvlj2RJtJwP/boosyG
> =/EiT
> -----END PGP SIGNATURE-----

[Jan Safranek]

To sum up discussion in this thread and on IRC:

  * The Review Board instance on https://reviewboard-openlmi.rhcloud.com
    is broken, some javascript is missing and cannot be used.

  * We tested repositories on https://github.com/openlmi/

      * 'Pull request' interface is usable, but when someone wants to
        send new version of a patch, he/she must do so in a new branch
        and create new pull request. (it's fine for me).
          o He/she *must not* push to the same branch, otherwise
            comments of the changed patches are lost without any
            notice/email.

      * Sending emails from github is suboptimal, especially sending
        copy to the mailing list. If I remember correctly, we would need
        to add dummy user with email address of the list.

So, we can either wait for fixed ReviewBoard, start using GitHub or use
plain emails (kernel-style). Any opinions?

Jan