The New Git Source Step Does Not Handle Changes From GerritChangeSource Correctly

[jroo]

The implementation from buildbot.steps.sources.Git handles review changes from GerritChangeSource? correctly but the new implementation from buildbot.steps.sources.git.Git does not.

The following bits should be in master.cfg for reproducing the issue:

from buildbot.changes.gerritchangesource import GerritChangeSource

c[ 'change_source' ] = [
  GerritChangeSource(
    gerritserver = 'gerrit.example.com',
    gerritport = 29418,
    username = 'gerrit-user',
  ),
]

# ....

c['schedulers'] = [
  AnyBranchScheduler(
    name = 'all',
    change_filter = filter.ChangeFilter( project = 'MyProject' ),
    treeStableTimer = None,
    builderNames = [
      'Some Builder',
    ],
  ),
]

# ...

factory = BuildFactory()
factory.addStep( Git(
  repourl = GitRepoUrl,
  mode = 'update',
  submodules = True,
  )
)

c[ 'builders' ] = [

  BuilderConfig(
    name = 'Some Builder',
    slavenames = [ 'Slave', ],
    mergeRequests = False,
    factory = factory,
  ),
]

[dustin]

There's some code to handle gerrit-specific properties in oldsource.py

    def startVC(self, branch, revision, patch):
        self.args['branch'] = branch
        self.args['repourl'] = self.repourl
        self.args['revision'] = revision
        self.args['patch'] = patch

        # check if there is any patchset we should fetch from Gerrit
        if self.build.hasProperty("event.patchSet.ref"):
            # GerritChangeSource
            self.args['gerrit_branch'] = self.build.getProperty("event.patchSet.ref") 
            self.updateSourceProperty("gerrit_branch",
                    self.args['gerrit_branch'])
        else:
            try:
                # forced build
                change = self.build.getProperty("gerrit_change", '').split('/')
                if len(change) == 2:
                    self.args['gerrit_branch'] = "refs/changes/%2.2d/%d/%d" \
                                                 % (int(change[0]) % 100, int(change[0]), int(change[1])) 
                    self.updateSourceProperty("gerrit_branch",
                            self.args['gerrit_branch'])
            except:
                pass

I'm not sure what the advantage is of supporting this in the Git step rather than the Gerrit step.

[jroo]

In my use case it does not matter if Git and Gerrit source steps are implemented in different classes. For some it might; someone could have GitPoller? and GerritChangeSource? objects as change source and if the source fetch steps would be in different classes the build steps would be required to be different depending on the source of the change.

[dustin]

Right, my point is just that you wouldn't expect a GitPoller and an SVN step to work together, so I'm not sure why GerritChangeSource and Git should work together, rather than GerritChangeSource and Repo, or a new Gerrit class (that may be a subclass of Git). I'm really just looking for the most appropriate design.

[jroo]

But Gerrit implements Git repository so you might expect Git and Gerrit to work together.

In my opinion it would be good solution to just inherit the gerrit step from git step if not implement both of those in single class; after all Gerrit is a Git repository + something else.

[dustin]

Fair enough. I'd prefer to see this as a subclass, when it gets implemented.

[cbessette]

Using ​https://raw.github.com/buildbot/buildbot/5a2b8ee0b559712d6413100aeee49a42ec5cd0cf/master/buildbot/steps/source/gerrit.py with Buildbot 0.8.7p1 worked for me.

[jroo]

I tested the new gerrit step and it seems to work fine for updates. But if the build slave clones (i.e. when the build slave runs for the first time) and the change is due to new review the slave does not get the correct commit for building.

The following git clone is run: git clone --branch refs/changes/60/10760/2 <my-git-repo-url> .

And the following warning is issued by git: warning: Remote branch refs/changes/60/10760/2 not found in upstream origin, using HEAD instead

This is because the refs/changes/xx/yyy is not a branch and thus does not work for clone command.

[dustin]

That sounds like #2548