Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#2008 closed defect (fixed)

mergeRequests globally or per builder seems to not function

Reported by: bdbaddog Owned by:
Priority: major Milestone: 0.8.5
Version: 0.8.4 Keywords:
Cc:

Description (last modified by dustin)

I have an example where one builder fires off 20 triggers into another builder. The global c['mergeRequests'] is set to False (or a function which returns false) and it still merges the builds.

I'll attach example master.cfg

Attachments (1)

master_badMerging.cfg (3.4 KB) - added by bdbaddog 9 years ago.
master.cfg which demonstrates issue.

Download all attachments as: .zip

Change History (12)

Changed 9 years ago by bdbaddog

master.cfg which demonstrates issue.

comment:1 Changed 9 years ago by dustin

  • Description modified (diff)
  • Keywords trigger mergeRequest removed
  • Milestone changed from undecided to 0.8.5

comment:2 Changed 9 years ago by bdbaddog

  • Milestone changed from 0.8.5 to undecided

Partial fix here: https://github.com/bdbaddog/buildbot/commit/6ebcfa982c47706002151e2e29c25d9fffd815df

Still has a lot of my debugging code. This only addresses the mergeRequests arg to BuildConfig() and not the global.

comment:4 Changed 9 years ago by dustin

  • Milestone changed from undecided to 0.8.5

comment:5 Changed 9 years ago by Dustin J. Mitchell

remove unused BotMaster?.shouldMergeRequests

Refs #2008

Changeset: 29c9de940d34a0de8268f83a3b7b4f4229d25657

comment:6 Changed 9 years ago by Dustin J. Mitchell

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

Builder configuration should default mergeRequests to None, not True

Without this change, mergeRequests is True regardless of the global setting, unless specifically set to something else in the BuilderConfig?.

This also fixes an error in _getMergeRequestFn, which was looking at self.master.mergeRequests, not self.botmaster.mergeRequests, and updates the tests to match and to have somewhat more readable inputs.

Fixes #2008.

Changeset: 984c004a6b04d616e727f6d984e179126fdc9cde

comment:7 Changed 9 years ago by Dustin J. Mitchell

Allow cmergeRequests? = True

This is the default, and therefore unnecessary, but it shouldn't be explicitly disallowed. Refs #2008.

Changeset: 7928163ee56f657ea7bbd3791af5271ac04a87af

comment:8 Changed 9 years ago by Dustin J. Mitchell

remove unused BotMaster?.shouldMergeRequests

Refs #2008

Changeset: f44c2e9fc13cda9e0e15f6bb4daf72cc7906dafd

comment:9 Changed 9 years ago by Dustin J. Mitchell

Builder configuration should default mergeRequests to None, not True

Without this change, mergeRequests is True regardless of the global setting, unless specifically set to something else in the BuilderConfig?.

This also fixes an error in _getMergeRequestFn, which was looking at self.master.mergeRequests, not self.botmaster.mergeRequests, and updates the tests to match and to have somewhat more readable inputs.

Fixes #2008.

Changeset: 977eb41f40215a65215b6e74beac66a9f46b1801

comment:10 Changed 9 years ago by Dustin J. Mitchell

Allow cmergeRequests? = True

This is the default, and therefore unnecessary, but it shouldn't be explicitly disallowed. Refs #2008.

Changeset: 88931660eb8d30d9f4515d6ae7b23486d9f9c15b

comment:11 Changed 9 years ago by dustin

The actual fix here is in [984c004a6b04d616e727f6d984e179126fdc9cde], and is the one you had identified - an incorrect default value used when getting {{{mergeRequests}} from the builder's config dictionary. My patch reverts behavior to the previous behavior, rather than changing documentation and tests to match new (incorrect) behavior.

Note: See TracTickets for help on using tickets.