Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#2485 closed defect (fixed)

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

Reported by: jroo Owned by:
Priority: major Milestone: 0.8.+
Version: 0.8.7p1 Keywords: gerrit git master-side-source-steps
Cc:

Description

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,
  ),
]


Change History (9)

comment:1 Changed 6 years ago by dustin

  • Keywords master-side-source-steps added
  • Milestone changed from undecided to 0.8.+

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.

comment:2 Changed 6 years ago by 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.

comment:3 Changed 6 years ago by 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.

comment:4 Changed 6 years ago by 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.

comment:5 Changed 6 years ago by dustin

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

comment:6 Changed 6 years ago by srinup

  • Resolution set to fixed
  • Status changed from new to closed

comment:8 Changed 6 years ago by 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.

comment:9 Changed 6 years ago by dustin

That sounds like #2548

Note: See TracTickets for help on using tickets.