-
Notifications
You must be signed in to change notification settings - Fork 39
Alternative rollup implementation for the rust branch #47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| /bors.cfg | ||
| /bors.log | ||
| /bors-status.js | ||
| *.pyc |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -227,6 +227,7 @@ def __init__(self, cfg, gh, j): | |
| self.user = cfg["gh_user"].encode("utf8") | ||
| self.test_ref = cfg["test_ref"].encode("utf8") | ||
| self.master_ref = cfg["master_ref"].encode("utf8") | ||
| self.batch_ref = cfg.get('batch', 'batch') | ||
| self.reviewers = [ r.encode("utf8") for r in cfg["reviewers"] ] | ||
| self.num=j["number"] | ||
| self.dst_owner=cfg["owner"].encode("utf8") | ||
|
|
@@ -314,8 +315,15 @@ def approval_list(self): | |
| for (_,_,c) in self.head_comments | ||
| for m in [re.match(r"^r=(\w+)", c)] if m ]) | ||
|
|
||
| def batched(self): | ||
| for date, user, comment in self.head_comments: | ||
| if re.search(r'\b(?:rollup|batch)\b', comment): | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What exactly is this regexp doing? Is it looking for either 'rollup' or 'batch'? If so, why? What does '?:' do?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it searches There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd recommend adding a comment explaining the goal of the regex regardless. regexes are great, but a total comprehension blackhole. |
||
| return True | ||
| return False | ||
|
|
||
| def priority(self): | ||
| p = 0 | ||
| p = -1 if self.batched() else 0 | ||
|
|
||
| for (d, u, c) in self.head_comments: | ||
| m = re.search(r"\bp=(-?\d+)\b", c) | ||
| if m != None: | ||
|
|
@@ -437,6 +445,20 @@ def reset_test_ref_to_master(self): | |
| self.dst().git().refs().heads(self.test_ref).patch(sha=master_sha, | ||
| force=True) | ||
|
|
||
| def parse_merge_sha(self): | ||
| parsed_merge_sha = None | ||
|
|
||
| for s in self.dst().statuses(self.sha).get(): | ||
| if s['creator']['login'].encode('utf-8') == self.user and s['state'].encode('utf-8') == 'pending': | ||
| mat = re.match(r'running tests for.*?candidate ([a-z0-9]+)', s['description'].encode('utf-8')) | ||
| if mat: parsed_merge_sha = mat.group(1) | ||
| break | ||
|
|
||
| if self.merge_sha: | ||
| assert self.merge_sha == parsed_merge_sha | ||
| else: | ||
| self.merge_sha = parsed_merge_sha | ||
|
|
||
| def merge_pull_head_to_test_ref(self): | ||
| s = "merging %s into %s" % (self.short(), self.test_ref) | ||
| try: | ||
|
|
@@ -455,14 +477,85 @@ def merge_pull_head_to_test_ref(self): | |
| self.merge_sha) | ||
| self.log.info(s) | ||
| self.add_comment(self.sha, s) | ||
| self.set_pending("running tests", u) | ||
| self.set_pending("running tests for candidate {}".format(self.merge_sha), u) | ||
|
|
||
| except github.ApiError: | ||
| s = s + " failed" | ||
| self.log.info(s) | ||
| self.add_comment(self.sha, s) | ||
| self.set_error(s) | ||
|
|
||
| def merge_batched_pull_reqs_to_test_ref(self, pulls): | ||
| batch_msg = 'merging {} batched pull requests into {}'.format( | ||
| len([x for x in pulls if x.current_state() == STATE_APPROVED]), | ||
| self.batch_ref, | ||
| ) | ||
| self.log.info(batch_msg) | ||
| self.add_comment(self.sha, batch_msg) | ||
|
|
||
| info = self.dst().git().refs().heads(self.master_ref).get() | ||
| master_sha = info['object']['sha'].encode('utf-8') | ||
| try: | ||
| self.dst().git().refs().heads(self.batch_ref).get() | ||
| self.dst().git().refs().heads(self.batch_ref).patch(sha=master_sha, force=True) | ||
| except github.ApiError: | ||
| self.dst().git().refs().post(sha=master_sha, ref='refs/heads/' + self.batch_ref) | ||
|
|
||
| successes = [] | ||
| failures = [] | ||
|
|
||
| batch_sha = '' | ||
|
|
||
| for pull in pulls: | ||
| if pull.current_state() == STATE_APPROVED: | ||
| self.log.info('merging {} into {}'.format(pull.short(), self.batch_ref)) | ||
|
|
||
| msg = 'Merge pull request #{} from {}/{}\n\n{}\n\nReviewed-by: {}'.format( | ||
| pull.num, | ||
| pull.src_owner, pull.ref, | ||
| pull.title, | ||
| ', '.join(pull.approval_list()) | ||
| ) | ||
| pull_repr = '- {}/{} = {}: {}'.format(pull.src_owner, pull.ref, pull.sha, pull.title) | ||
|
|
||
| try: | ||
| info = self.dst().merges().post(base=self.batch_ref, head=pull.sha, commit_message=msg) | ||
| batch_sha = info['sha'].encode('utf-8') | ||
| except github.ApiError: | ||
| failures.append(pull_repr) | ||
| else: | ||
| successes.append(pull_repr) | ||
|
|
||
| if batch_sha: | ||
| try: | ||
| self.dst().git().refs().heads(self.test_ref).get() | ||
| self.dst().git().refs().heads(self.test_ref).patch(sha=batch_sha) | ||
| except github.ApiError as e: | ||
| self.dst().git().refs().post(sha=batch_sha, ref='refs/heads/' + self.test_ref) | ||
|
|
||
| url = 'https://github.com/{}/{}/commit/{}'.format(self.dst_owner, self.dst_repo, batch_sha) | ||
| short_msg = 'running tests for rollup candidate {} ({} successes, {} failures)'.format(batch_sha, len(successes), len(failures)) | ||
| msg = 'Testing rollup candidate = {:.8}'.format(batch_sha) | ||
| if successes: msg += '\n\n**Successful merges:**\n\n{}'.format('\n'.join(successes)) | ||
| if failures: msg += '\n\n**Failed merges:**\n\n{}'.format('\n'.join(failures)) | ||
|
|
||
| self.log.info(short_msg) | ||
| self.add_comment(self.sha, msg) | ||
| self.set_pending(short_msg, url) | ||
| else: | ||
| batch_msg += ' failed' | ||
|
|
||
| self.log.info(batch_msg) | ||
| self.add_comment(self.sha, batch_msg) | ||
| self.set_error(batch_msg) | ||
|
|
||
| def merge_or_batch(self, pulls): | ||
| self.reset_test_ref_to_master() | ||
| if self.batched(): | ||
| self.merge_batched_pull_reqs_to_test_ref(pulls) | ||
| else: | ||
| self.merge_pull_head_to_test_ref() | ||
|
|
||
| def advance_master_ref_to_test(self): | ||
| assert self.merge_sha != None | ||
| s = ("fast-forwarding %s to %s = %.8s" % | ||
|
|
@@ -487,7 +580,7 @@ def advance_master_ref_to_test(self): | |
|
|
||
|
|
||
|
|
||
| def try_advance(self): | ||
| def try_advance(self, pulls): | ||
| s = self.current_state() | ||
|
|
||
| self.log.info("considering %s", self.desc()) | ||
|
|
@@ -504,17 +597,16 @@ def try_advance(self): | |
| self.src_repo, | ||
| self.sha)))) | ||
|
|
||
| self.reset_test_ref_to_master() | ||
| self.merge_pull_head_to_test_ref() | ||
| self.merge_or_batch(pulls) | ||
|
|
||
| elif s == STATE_PENDING: | ||
| self.parse_merge_sha() | ||
| if self.merge_sha == None: | ||
| c = ("No active merge of candidate %.8s found, likely manual push to %s" | ||
| % (self.sha, self.master_ref)) | ||
| self.log.info(c) | ||
| self.add_comment(self.sha, c) | ||
| self.reset_test_ref_to_master() | ||
| self.merge_pull_head_to_test_ref() | ||
| self.merge_or_batch(pulls) | ||
| return | ||
| self.log.info("%s - found pending state, checking tests", self.short()) | ||
| assert self.merge_sha != None | ||
|
|
@@ -548,6 +640,7 @@ def try_advance(self): | |
|
|
||
| elif s == STATE_TESTED: | ||
| self.log.info("%s - tests successful, attempting landing", self.short()) | ||
| self.parse_merge_sha() | ||
| self.advance_master_ref_to_test() | ||
|
|
||
|
|
||
|
|
@@ -717,7 +810,7 @@ def main(): | |
| else: | ||
| p = pulls[-1] | ||
| logging.info("working with most-ripe pull %s", p.short()) | ||
| p.try_advance() | ||
| p.try_advance(list(reversed(pulls))) | ||
|
|
||
|
|
||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this
cfg.getwhen the surround code is not?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I guess it's a default value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think providing a sane default when the configuration value does not exist is good. (That's what
cfg.get(key, default_value)does.) So I think we might even want to replace almost all the occurrences ofcfg[key]tocfg.get(key, default_value).However, it is just a matter of taste. You could also prefer explicit failing if the configuration value is not provided. Which way do you prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine as is, thanks.