Open Stack

Perhaps sending that email right before taking a few days off meaning I
couldn't reply straight away wasn't the most helpful ;-)


On Thu, 5 Jul 2018, 02:57 Jeremy Stanley, <fungi at yuggoth.org> wrote:

> On 2018-07-04 22:32:53 +0100 (+0100), Darragh Bailey wrote:
> [...]
> > Based on the comments at
> >
> https://git.openstack.org/cgit/openstack-infra/git-review/tree/CONTRIBUTING.rst#n5
> ,
> > git-review is considered feature complete, and as a consequence it
> > seems that reviewers have mostly moved onto other projects so it
> > can take quite some time to get reviews. Perfectly understandable,
> > everyone can only do so much and needs to pick something(s) to
> > prioritise. However this is such a useful tool for working with
> > Gerrit from the command line it seems to be the defacto git
> > subcommand for interfacing with Gerrit that it seems a shame to
> > limit it.
>
> At one time git-review had started to suffer from scope creep and
> was getting more random proposals for new features than actual
> stability improvements. Its test coverage was, for quite a long
> while, also sub-par so that some of the feature additions which did
> get accepted introduced regressions that went unnoticed sometimes
> for weeks or months before we'd discover they needed reverting. The
> original authors intended for git-review to be primarily focused on
> bootstrapping Gerrit connectivity from a cloned Git repository as
> well as simplifying the basic Git commands for retrieving changes
> from and pushing changes to a Gerrit. That update to the
> CONTRIBUTING.rst file was intended to put the brakes on future scope
> creep, especially in cases where an added feature would work just as
> well as its own separate git subcommand.
>

You've made one suggestion below on that applying for the list behaviour.
Made me think that an etherpad listing the current outstanding or abandoned
changes and discussing which would be better off outside of git-review
would be a useful starting place.

> While I think there are a number of current reviews that would be
> > beneficial to git-review, as well as some pieces that don't appear
> > to be there currently, I'm reluctant to invest much time as it
> > seems unlikely enhancements would be accepted due to the current
> > state of feature complete. Instead of putting together various
> > changes to see if they might be reviewed and accepted, hoping a
> > chat about what paths might be available could save a bit of time.
>
> I've tried to go in and approve changes from time to time, but in
> all honesty the negativity I've received in the past when attempting
> to push back on feature additions has caused me to deprioritize
> reviewing more changes for it. I should probably just buck up and go
> in with a (very polite) machete anyway.
>

I think it's all due to needing to prioritise time spent and git-review has
mostly done what's needed.


> There are a couple of things that I would like to work towards:
> >
> > * Change the tests to use a single gerrit with separate projects
> > instead of separate instances (faster testing)
>
> This seems reasonable if it doesn't introduce new races or odd test
> interdependencies from the reduced fixture isolation. I really have
> never been fond of the integration-testing-only model we ended up
> with though. I originally recommended lower-level unit testing with
> mocks for the Git and SSH interactions, but the one volunteer we got
> to implement a testsuite chose to automate Gerrit installation so it
> is what it is at the moment.
>

More mocks around ssh/http should work well, but I've found that it's not
necessarily beneficial doing the same around git, as with some simple
fixtures it can be tested very quickly.

The existing tests are still useful, and changing to a single gerrit
instance per runner would require some thought in the logging side (adding
markers should be sufficient I think), but thetas probably the main issue.


> * Allow the tests to run against multiple versions of Gerrit (ensure
> > compatibility)
>
> This seems reasonable. We should have been bumping the Gerrit
> versions in the tests and/or running more jobs for different
> releases of it but the way version selection was implemented would
> need a bit of an overhaul to accommodate that.
>

I'm thinking also for compatibility across a few releases would be good as
well.

> * Fix and land many of the changes making it easier to download
> > changes, list changes ordered with their dependencies, stashing
> > when downloading, etc
>
> The change listing feature really seems increasingly out of place to
> me, and most of the "fixes" I saw related to it were about
> supporting more and more of Gerrit's query language and terminal
> formatting. If we deprecated/removed that and recommended
> interacting directly with Gerrit or alternative utilities for change
> searches (there are a lot more options for this than there were back
> when git-review was first written) all of those would become
> unnecessary and the code would be simplified at the same time.
>

That's interesting, I'd consider the ability to query for what is available
for review a step before downloading a change for review, and understanding
that it might be bringing multiple chances down that aren't merged useful.

If this was moved out of git-review, I suspect it might still need to know
a bit about git-review and be able to use some of its configuration.


> * Have git-review auto configure refs/notes/review (assuming it's
> > available) for fetching on setup (I find it very handy and I'm
> > always forgetting to do this)
>
> I could see this being in scope, as it fits with the Gerrit
> connectivity bootstrapping mission. I too find the notes refs handy
> but have a global configuration in my ~/.gitconfig which seems to do
> the trick already so I'm curious to find out how git-review might
> improve on that.
>

I didn't think that this could be done for the 'origin' remote and would be
ignored by fit for other projects where it doesn't exist? Or are you using
the 'gerrit' remote?

But to the main advantage is that it opens this up to many people that
might not have been aware it exists. And also opens up to asking the user
if they'd like the review notes displayed along with the logs by default
for this repo by setting core.notesRef.

> And potentially controversially; support other workflows and
> > options outside of the OpenStack workflow. Although maybe not
> > directly, and still keeping the OpenStack one as the default.
>
> I'd love to know what about git-review is focused on OpenStack's
> workflow. We tried to make it as generic as possible. If there are
> any OpenStack-specific features still lingering in there, we should
> see about ripping them out as soon as is feasible. One that I'm
> aware of is the default topic mangling based on commit message
> parsing, which I've been wanting to eradicate for a while since
> Gerrit now makes altering topics possible without needing to push a
> new commit. For that matter, setting the topic based on the local
> branch name could also get tossed while we're at it, and just keep
> the -t option for directly specifying a change topic when people
> really want to do it at time of upload.
>

There may be others, I recall this coming up before around being able to
set review scores for labels at the same time as uploading the change.
Think it was around the 'Workflow' label. Maybe this is a case for a
different command, but it seems likely to break the flow doe anyone using
git-review to submit. However the labels for each project are customizable
so it seems likely this would need the correct set to be worked out at
setup time if included.

> I think there are a couple of ways that could be achieved, but I
> > can't see any of them working well without a decent amount of
> > refactoring.
> >
> > * Have git-review provide the APIs so that someone may define a
> > git-review-<name> that can add their workflow
> >
> > * Add support for additional behaviour to be defined with
> > refs/meta/config of projects
> >
> > * Allow extensions to be installed that allow additional options
> > to be added to the git-review CLI and config file
> >
> > That last one might require being able to specify the additional
> > required plugins to be listed in .gitreview, and providing the
> > documentation might be trickier?
> >
> > Basically make it easier to add custom behaviour without it being
> > builtin to git-review, and without needing to reimplement a whole
> > load of functionality elsewhere. But I'm pretty sure that all
> > requires a substantial rewrite.
>
> I'd need some concrete use case examples. From my perspective,
> git-review is already a plugin (by way of being a git subcommand) so
> adding plugins to the plugin seems like a layer violation. The
> examples I've seen in the past for adding new behaviors were things
> which made more sense to me as new git subcommands. For a
> counterexample, James Blair created git-restack not too long ago...
> it could have been implemented as a git-review option, but was
> sanely made to be its own distinct git subcommand instead.
>

Maybe the list one suggested to be outside of git-review. I suspect it'd
still want to be connected to it in some way.


> > Thoughts? Is it worth putting a plan together around some of the
> > initial changes? And then revisiting what would be needed to allow
> > extensions around other workflows?
>
> I'm all for plans to improve git-review's stability, test coverage
> and, most of all, simplicity. Thanks for raising the topic!
> --
> Jeremy Stanley


Typing this all on a mobile so might have missed a few things and will
follow up after the weekend.


--
Darragh Bailey
"Nothing is foolproof to a sufficiently talented fool" - unknown

>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-infra/attachments/20180706/27a7e749/attachment-0001.html>