Differential D1183

changing name of 'branch' parameter so it isn't squashed in buildbot. Fixes T933
ClosedPublic
Actions

Authored by tflink on Mar 29 2017, 9:45 PM.

Details

Reviewers
mkrizek
jskladan
Commits
rTRGRaac5a5ef821d: changing name of 'branch' parameter so it isn't squashed in buildbot. Fixes T933
Summary

Simple change to use a parameter name that isn't squashed by buildbot

fixing arcconfig so it doesn't blow up

Test Plan

run trigger, see that 'branch' has been replaced by 'distgit_branch'. This code is hacked into stg ATM

Diff Detail

Repository
rTRGR taskotron-trigger
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
tflink created this revision.Mar 29 2017, 9:45 PM
jskladan requested changes to this revision.Mar 30 2017, 8:01 AM

Apart of the comments below, I'd like to see the testsuite updated to reflect the changes, I don't see how this could pass unittests.

.arcconfig
6 ↗(On Diff #3006)

Is this really supposed to be removed? Not that I understand the arcconfig that much, but does this not plain disable pytest?

jobtriggers/jobtrigger.py
83

Since this is not really dist-git specific, I'd rather have the name be git_branch, and changed the repo to git_repo for the sake of consistency.

This revision now requires changes to proceed.Mar 30 2017, 8:01 AM
tflink planned changes to this revision.Mar 30 2017, 2:17 PM
tflink added inline comments.
.arcconfig
6 ↗(On Diff #3006)

I'm really not sure how I came to the conclusion but I was convinced that there was no test suite and that this needed to be removed in order to submit the diff.

Thanks for catching this, will fix

jobtriggers/jobtrigger.py
83

sounds good

tflink updated this revision to Diff 3007.Mar 30 2017, 2:25 PM
  • fixing test suite, addressing comments in review
mkrizek added a comment.Apr 3 2017, 7:00 AM

Looks good, but leaving to @jskladan for ack.

jskladan accepted this revision.Apr 3 2017, 1:40 PM

I'd like to have the task['repo'] changed to task['git_repo'] for the sake of consistency, but it is not a huge issue. Looks good other than this nitpick.

This revision is now accepted and ready to land.Apr 3 2017, 1:40 PM
tflink updated this revision to Diff 3008.Apr 3 2017, 1:45 PM
  • changing repo to git_repo per review
Closed by commit rTRGRaac5a5ef821d: changing name of 'branch' parameter so it isn't squashed in buildbot. Fixes T933 (authored by tflink). · Explain WhyApr 4 2017, 2:21 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathPackages
Mjobtriggers/jobtrigger.py (6 lines)
Mtesting/test_jobtrigger.py (8 lines)

Diff 3013

View Options

jobtriggers/jobtrigger.py

Show First 20 Lines • Show All 72 Lines • ▼ Show 20 Line(s)67 elif 'discover' in item:
73 fallback = discover.get('fallback_branch', '').strip() 73​ fallback = discover.get('fallback_branch', '').strip()
74 repo_dir, branch = utils.clone_repo(repo, branch, fallback) 74​ repo_dir, branch = utils.clone_repo(repo, branch, fallback)
75 75​
76 # append base_dir, if specified 76​ # append base_dir, if specified
77 base_dir = discover.get('base_dir', '').strip() 77​ base_dir = discover.get('base_dir', '').strip()
78 repo_dir = os.path.join(repo_dir, base_dir) 78​ repo_dir = os.path.join(repo_dir, base_dir)
79 # fill the task details 79​ # fill the task details
80 recursive = discover.get('recursive', False) 80​ recursive = discover.get('recursive', False)
81 task['repo'] = repo 81​ task['git_repo'] = repo
82 task['branch'] = branch 82​ # turns out that buildbot effectively stomps on this param if we name it 'branch'
83​ # see T933 for details
jskladanUnsubmitted
Not Done

Since this is not really dist-git specific, I'd rather have the name be git_branch, and changed the repo to git_repo for the sake of consistency.

tflinkAuthorUnsubmitted
Not Done

sounds good

84​ task['git_branch'] = branch
83 task['tasks'] = utils.discover_tasks(repo_dir, task['item_type'], recursive) 85​ task['tasks'] = utils.discover_tasks(repo_dir, task['item_type'], recursive)
84 else: 86​ else:
85 self.log.error("Rule has invalid `do` section: %r", rule) 87​ self.log.error("Rule has invalid `do` section: %r", rule)
86 88​
87 tasks.append(task) 89​ tasks.append(task)
88 90​
89 return tasks 91​ return tasks
90 92​
Show All 39 Lines
View Options

testing/test_jobtrigger.py

Show First 20 Lines • Show All 79 Lines • ▼ Show 20 Line(s)20class TestJobtrigger():
80 def test__tasks_from_rule_discover(self): 80​ def test__tasks_from_rule_discover(self):
81 ref_repo = 'http://bogus.repo/firefox.git' 81​ ref_repo = 'http://bogus.repo/firefox.git'
82 rule = {'do': [{'discover': {'repo': ref_repo}}]} 82​ rule = {'do': [{'discover': {'repo': ref_repo}}]}
83 tasks = self.helper._tasks_from_rule(rule, self.ref_message_data) 83​ tasks = self.helper._tasks_from_rule(rule, self.ref_message_data)
84 assert len(tasks) == 1 84​ assert len(tasks) == 1
85 assert tasks[0]['tasks'] == self.ref_discovered_tasks 85​ assert tasks[0]['tasks'] == self.ref_discovered_tasks
86 assert tasks[0]['item'] == self.ref_message_data['item'] 86​ assert tasks[0]['item'] == self.ref_message_data['item']
87 assert tasks[0]['item_type'] == self.ref_message_data['item_type'] 87​ assert tasks[0]['item_type'] == self.ref_message_data['item_type']
88 assert tasks[0]['repo'] == ref_repo 88​ assert tasks[0]['git_repo'] == ref_repo
89 assert tasks[0]['branch'] == 'master' 89​ assert tasks[0]['git_branch'] == 'master'
90 90​
91 def test__tasks_from_rule_discover_set_branch(self): 91​ def test__tasks_from_rule_discover_set_branch(self):
92 ref_repo = 'http://bogus.repo/firefox.git' 92​ ref_repo = 'http://bogus.repo/firefox.git'
93 rule = {'do': [{'discover': {'repo': ref_repo, 'branch': 'mybranch'}}]} 93​ rule = {'do': [{'discover': {'repo': ref_repo, 'branch': 'mybranch'}}]}
94 tasks = self.helper._tasks_from_rule(rule, self.ref_message_data) 94​ tasks = self.helper._tasks_from_rule(rule, self.ref_message_data)
95 assert len(tasks) == 1 95​ assert len(tasks) == 1
96 assert tasks[0]['tasks'] == self.ref_discovered_tasks 96​ assert tasks[0]['tasks'] == self.ref_discovered_tasks
97 assert tasks[0]['item'] == self.ref_message_data['item'] 97​ assert tasks[0]['item'] == self.ref_message_data['item']
98 assert tasks[0]['item_type'] == self.ref_message_data['item_type'] 98​ assert tasks[0]['item_type'] == self.ref_message_data['item_type']
99 assert tasks[0]['repo'] == ref_repo 99​ assert tasks[0]['git_repo'] == ref_repo
100 assert tasks[0]['branch'] == 'mybranch' 100​ assert tasks[0]['git_branch'] == 'mybranch'
101 101​
102 def test__tasks_from_rule_discover_recursive(self, monkeypatch): 102​ def test__tasks_from_rule_discover_recursive(self, monkeypatch):
103 utils_dingus = Dingus(discover_tasks__returns=self.ref_discovered_tasks) 103​ utils_dingus = Dingus(discover_tasks__returns=self.ref_discovered_tasks)
104 monkeypatch.setattr(utils, 'discover_tasks', utils_dingus.discover_tasks) 104​ monkeypatch.setattr(utils, 'discover_tasks', utils_dingus.discover_tasks)
105 105​
106 ref_repo = 'http://bogus.repo/firefox.git' 106​ ref_repo = 'http://bogus.repo/firefox.git'
107 rule = {'do': [{'discover': {'repo': ref_repo, 'recursive': True}}]} 107​ rule = {'do': [{'discover': {'repo': ref_repo, 'recursive': True}}]}
108 self.helper._tasks_from_rule(rule, self.ref_message_data) 108​ self.helper._tasks_from_rule(rule, self.ref_message_data)
▲ Show 20 Lines • Show All 62 Lines • Show Last 20 Lines