openstack team mailing list archive

[Monty Taylor <mordred@xxxxxxxxxxxx>]

On 08/09/2011 01:28 PM, Jay Pipes wrote:
> On Tue, Aug 9, 2011 at 1:01 PM, Dan Prince <dan.prince@xxxxxxxxxxxxx> wrote:
>> Couple of comments:
>>
>> 1) While gerrit is integrated w/ Launchpad (and can close tickets) Launchpad
>> is not integrated with Gerrit. Things like referencing a branch from within
>> a ticket or blueprint aren't going to work as well as they used to right?

This is correct (although it's certainly something that we could look in
to over time - launchpad is open source and their stated goal is
integration with upstream projects - so perhaps expanding branch links
to include external branches is something possible - but certainly not
in the next six months)

> Actually, this is being worked on:
> https://bugs.launchpad.net/openstack-ci/+bug/823173

Well, and there's that. :)

>> 2) I'd like to see a unified diff containing all the files on the Gerrit
>> review pages. Is there a way to do this or am I missing something?
> 
> Also being worked on, by the Nokia team and should be in the next
> release of Gerrit:
> I think this is the feature issue:
> http://code.google.com/p/gerrit/issues/detail?id=106
> 
> That being said, after using the "next changed source file" links,
> I've actually changed my mind about using a single diff for larger
> commits with lots of files changed. Sometimes it's actually nice to
> scroll through them. But, having the option to have a single page diff
> would be useful at times, too...

I, for one, definitely want the single-page review- but also agree with
Jay, the next-file isn't as bad as I thought originally. (also, gerrit
is very ajax heavy- so most of those buttons/links are doing internal
state changes and not reloads - so the next page links should be snappy)

>> 3) The branch/refspec names Gerrit uses are not very user friendly. In
>> Launchpad we typically had people naming their branches w/ either a
>> feature/fix name or the ticket number. So in Launchpad my branch would be
>> called something like 'lp:~dan-prince/fix_ec2_metadata' or whatever. In
>> Gerrit the branch names up for review are rather cryptic
>> 'refs/changes/76/176/1' which means that when trying to track and Gate
>> branches before they hit trunk we are going to manually have to do an extra
>> bit of detective work to make sense of which tickets and features a
>> particular refspec corresponds to. Some extra tooling might might this
>> easier but I really dislike that we have no control over the branch names
>> that are up for review.
> 
> I will let Jim and Monty talk about this.

Well- there are some interrelated things here. I'm going to try to talk
about them sensibly - but may ramble - excuse me for that please. :)

One is the branch/refspec names. In launchpad, there are named branches
someone pushes (like dan-prince/fix_ec2_metadata) - then there are
possible bug tags (commit --fixes=lp:213413) and then there are merge
prop descriptions and commit messages. When we rolled launchpad out
originally, there was a decent sized complaint that a merge prop would
have an additional comment.

In gerrit - there are no extra descriptions or commit messages because
it uses the commit message from git. Additionally - it uses the first
line of the commit message as a commit "subject" (which is a git common
practice) so that the list of pending changes actually has a descriptive
identifier if you wrote a good commit message.

So from a "look at the list of things to review" standpoint, I actually
feel like I have a much _clearer_ understanding of what each thing is
before I review it than I did just looking a list of branch names on
launchpad.

>From the perspective of associating proposed changes with
bugs/blueprints - we currently have a gerrit hook which will do regex
matching on commit messages to look for bug numbers and link things to
the launchpad bug. So as long as you say something like "Bug 235153:
Fixed ec2 problem" - this should work both for automatic scriptable
linking and user semantic understanding.

The key here is that the branch name or refspec isn't the list you
should be looking for identifying info - it's the commit subject (which
also means less duplication of effort on a dev's side) If something is
wanting to process this that is not jenkins (such as smokestack) ... the
commit message is in the json object returned by the gerrit query
command - so it should be fairly simple to do the same or similar
parsing to show a description of the branch being tested.

Is that helpful?

>> Lastly I kind of feel like we've bee dupped. When we talked about Git code
>> hosting on GitHub at the conference some of the major points were improved
>> code review (on GitHub) and performance improvements for checkouts, etc.
>> While we may have taken a step forward on the performance front IMHO we've
>> taken a major step backwards as far as the review and tracking process goes.
> 
> I explained the deficiencies in GitHub's pull request system for
> integration with an automated patch queue manager and gated trunk.
> Could you point to a review you've done in Gerrit so far that we can
> use as a starting point for discussion on what improvements can be
> made relative to the "GitHub process"? I couldn't find any reviews
> that you've done in Gerrit.

I certainly don't want anyone to feel duped here. We did start down the
road quite earnestly of using github, and could not meet all of the
requirements that we have currently as a project. At that point, we
moved on to attempt to solve the underlying problem that people had
expressed in the best way possible - which I'm sure you can understand
given the large number of people with conflicting interests was ... fun.

In looking at things - it seemed that, to be quite honest, the number
one single change that would make the maximum number of people happy was
switching from bzr to git, and there was nothing about git that would
not meet the needs of the project. (it is, of course, an excellent tool)

All in all, I'm not saying that all of the design choices are perfect,
and there are certainly things to work on - but I _do_ think that we're
in an excellent position now to actually effect the changes that we
need. (which will make it more effective to sit down at design summits
and discuss needs - since we should be able to actually implement them)

Thanks!
Monty

>> -----Original Message-----
>> From: "Jay Pipes" <jaypipes@xxxxxxxxx>
>> Sent: Monday, August 8, 2011 2:50pm
>> To: openstack@xxxxxxxxxxxxxxxxxxx
>> Subject: [Openstack] Status of Git/Gerrit Code Hosting/Review
>>
>> Hello all,
>>
>> tl;dr
>> =======
>>
>> Contributors have been giving Monty Taylor and Jim Blair feedback on
>> the Gerrit code review system over the last few weeks. Both the
>> Keystone and Glance projects have now migrated to using Git as their
>> source control system and Gerrit for code review and integration into
>> the Jenkins continuous integration system.
>>
>> Tomorrow, the Project Policy Board (PPB) will be voting on two things:
>>
>> 1) Should OS projects
>> a) have a vetted set of options for hosting and review, or
>> b) be required to use a single toolset for review and hosting
>> 2) Shall Gerrit+Git be included in the set of vetted options or be the
>> single option (dependent on the vote result for 1) above)
>>
>> Feedback on #2 is most welcome. Please feel free to respond to this
>> email, catch us on IRC or email me directly.
>>
>> Links:
>>
>> Working with Gerrit: http://wiki.openstack.org/GerritWorkflow
>> Code Review in Gerrit: http://review.openstack.org
>>
>> Details
>> =======
>>
>> Over the last few weeks, Monty Taylor and Jim Blair have been working
>> with a number of OpenStack contributors to gather feedback on a
>> Git-based development workflow, toolset, and review process.
>>
>> First, Monty and Jim investigated whether GitHub's pull request system
>> would be sufficient to enforce existing code review and approval
>> policies. It was determined that GitHub's pull request system was not
>> sufficient. The main reason why the pull request system failed to meet
>> needs is that there is no overall way to track the current state of a
>> given pull request. While this is fine for the simple case (merge
>> request is accepted and merged) it starts to fall over with some of
>> the more complex back and forths that we wind up having in many
>> OpenStack projects. Additionally, this assessment was predicated on
>> the current design of a gated trunk with an automated patch queue
>> manager, and a system where a developer is not required to spend time
>> landing a patch (other than potential needs for rebases or changes due
>> to code review).
>>
>> Monty and Jim then decided to set up a Gerrit server for code review
>> and CI integration at http://review.openstack.org. Gerrit is a tool
>> developed by Google to address some of the functionality the Android
>> Open Source team needed around automated patch queue management and
>> code reviews.
>>
>> The first project that moved from Launchpad to Gerrit/Git was the
>> openstack-ci project. This is the glue code and scripts that support
>> the continuous integration environment running on
>> http://jenkins.openstack.org.
>>
>> After gaining some experience with Gerrit through the migration of
>> this project from Launchpad, the next OpenStack subproject to move to
>> the Gerrit platform was the Keystone incubated project. Keystone was
>> already using git for source control and was on GitHub, using GitHub's
>> Issues for its pull requests and bug tracking. However, the Keystone
>> source code was not gated by a non-human patch queue management
>> system; a Keystone developer would manually merge proposed branches
>> into the master Keystone source tree, and code reviews were not passed
>> through any automated tests on http://jenkins.openstack.org. Monty and
>> Jim worked with Dolph, Yogi, Ziad, and other Keystone developers to
>> get their code reviews done via Gerrit and get their unit and
>> functional tests running on each commit through Jenkins. There were a
>> few hiccups along the way, but the hiccups served as valuable lessons
>> and were documented in the workflow wiki page
>> http://wiki.openstack.org/GerritWorkflow.
>>
>> Last Thursday morning, the Glance project was migrated from Bazaar and
>> Launchpad code hosting to use Git and Gerrit. The migration went
>> pretty smoothly, and a number of Glance developers have already been
>> proposing, reviewing, and approving code via Gerrit. Launchpad is used
>> for all bug tracking and blueprint management, still, but the code at
>> http://code.launchpad.net/glance is merely a read-only mirror of the
>> git repository.
>>
>> Outstanding Issues/Questions
>> =====================
>>
>> 1) John Dickinson and Chuck Thier raised the question that if Gerrit
>> is going to be the (or one of the) proposed code review and patch
>> management system, that hosting Git repositories on GitHub might be
>> confusing for GitHub users, since most would expect to use pull
>> requests to merge their own code back into the project's master repo.
>> This is a valid concern and Monty and Jim are investigating
>> establishing a GitWeb or Gitorious server on http://git.openstack.org
>> that would serve as the canonical Git repo locations for OpenStack
>> projects instead of GitHub. This would be similar to how
>> http://git.kernel.org works
>>
>> 2) Only code hosting has been moved to Git/Gerrit. There are currently
>> no plans to discuss moving bug tracking for existing OpenStack core
>> projects to GitHub Issues. Gerrit is fully integrated with Launchpad
>> bug tracking. This means that Gerrit *will close* (mark Fix Committed)
>> Launchpad bugs if you include bug text in your commit message.
>>
>> 3) The user interface for Gerrit is UGLY. I don't think anyone would
>> disagree with that. :) That said, Gerrit's UI can be modified via CSS
>> and templates without having to keep a separate fork of Gerrit. If you
>> are interested in helping Monty and Jim make the Gerrit UI prettier
>> and saving reviewers eyeballs, please do contact me.
>>
>> Cheers,
>> -jay
>>
>> _______________________________________________
>> Mailing list: https://launchpad.net/~openstack
>> Post to : openstack@xxxxxxxxxxxxxxxxxxx
>> Unsubscribe : https://launchpad.net/~openstack
>> More help : https://help.launchpad.net/ListHelp
>> This email may include confidential information. If you received it in
>> error, please delete it.
>>
>>
> 
> _______________________________________________
> Mailing list: https://launchpad.net/~openstack
> Post to     : openstack@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~openstack
> More help   : https://help.launchpad.net/ListHelp
>