sts-sponsors team mailing list archive

[Adam Collard <mp+437617@xxxxxxxxxxxxxxxxxx>]

Adam Collard has proposed merging ~adam-collard/maas-ci/+git/maas-ci-internal:tweak-job-reviewer into ~maas-committers/maas-ci/+git/maas-ci-internal:main.

Commit message:
[job-reviewer] unify testing criteria for performance



Requested reviews:
  MAAS Committers (maas-committers)

For more details, see:
https://code.launchpad.net/~adam-collard/maas-ci/+git/maas-ci-internal/+merge/437617
-- 
Your team MAAS Committers is requested to review the proposed merge of ~adam-collard/maas-ci/+git/maas-ci-internal:tweak-job-reviewer into ~maas-committers/maas-ci/+git/maas-ci-internal:main.

diff --git a/jobs/launchpad-ci/launchpad b/jobs/launchpad-ci/launchpad
index 9e4d9a0..1bd8927 100755
--- a/jobs/launchpad-ci/launchpad
+++ b/jobs/launchpad-ci/launchpad
@@ -93,27 +93,16 @@ def is_approved(proposal):
 
 
 def get_latest_commit_sha1(proposal):
-    for branch in proposal.source_git_repository.branches:
-        if branch.path == proposal.source_git_path:
-            return branch.commit_sha1
+    branch = proposal.source_git_repository.getRefByPath(path=proposal.source_git_path)
+    return branch.commit_sha1
 
 
 def extract_commit_from_comment(comment):
-    regex = r"COMMIT: (?P<sha1>\b[0-9a-f]{5,40}\b)"
+    regex = r"COMMIT: \b(?P<sha1>[0-9a-f]{5,40})\b"
     matches = re.search(regex, comment.message_body)
     if matches is None:
         return None
-    return matches.group("sha1").strip()
-
-
-def get_all_reviewed_commit_sha1(lp, proposal):
-    commits = []
-    for comment in proposal.all_comments:
-        if lp.me == comment.author:
-            commit = extract_commit_from_comment(comment)
-            if commit:
-                commits.append(commit)
-    return commits
+    return matches.group("sha1")
 
 
 def has_test_marker(comment):
@@ -123,19 +112,6 @@ def has_test_marker(comment):
     return False
 
 
-def should_unconditionally_test(lp, proposal):
-    hit_test_marker = False
-    for comment in proposal.all_comments:
-        if has_test_marker(comment):
-            hit_test_marker = True
-        if hit_test_marker and lp.me == comment.author:
-            commit = extract_commit_from_comment(comment)
-            if commit:
-                # Test marker was set but a following comment holds a test, so
-                # the test has already been ran for this proposal.
-                hit_test_marker = False
-    return hit_test_marker
-
 
 def generate_mergable_proposals(lp, git_repo):
     approved_proposals = list(git_repo.getMergeProposals(status="Approved"))
@@ -159,6 +135,37 @@ def generate_mergable_proposals(lp, git_repo):
 
         yield proposal
 
+def should_review(lp, proposal, repo_logger):
+    hit_test_marker = False
+    reviewed_commits = []
+    for comment in proposal.all_comments:
+        if has_test_marker(comment):
+            hit_test_marker = True
+        if lp.me == comment.author:
+            commit = extract_commit_from_comment(comment)
+            if commit:
+                reviewed_commits.append(commit)
+                if hit_test_marker:
+                    # Test marker was set but a following comment holds a test, so
+                    # the test has already been ran for this proposal.
+                    hit_test_marker = False
+    if hit_test_marker:
+        repo_logger.debug(f"Found marker on {proposal.web_link}; Testing!")
+        return True
+    # If WIP, see if this has been marked to be tested.
+    if proposal.queue_status == "Work in progress":
+        repo_logger.debug(f"Skipping {proposal.web_link}: WiP but no marker found")
+        return False
+    # See if the latest commit has already been reviewed.
+    latest_commit = get_latest_commit_sha1(proposal)
+    if latest_commit in reviewed_commits:
+        repo_logger.debug(
+            f"Skipping {proposal.web_link}: {latest_commit} has already been reviewed"
+        )
+        return False
+    else:
+        return True
+
 
 def generate_reviewable_proposals(lp, git_repo, repo_logger):
     needs_review_proposals = list(git_repo.getMergeProposals(status="Needs review"))
@@ -167,26 +174,9 @@ def generate_reviewable_proposals(lp, git_repo, repo_logger):
     shuffle(wip_proposals)
     for proposal in chain(needs_review_proposals, wip_proposals):
         repo_logger.debug(f"Considering {proposal.web_link}")
-        if should_unconditionally_test(lp, proposal):
-            repo_logger.debug(f"Found marker on {proposal.web_link}; Testing!")
+        if should_review(lp, proposal, repo_logger):
+            repo_logger.debug(f"Testing {proposal.web_link} !")
             yield proposal
-            continue
-
-        # See if the latest commit has already been reviewed.
-        latest_commit = get_latest_commit_sha1(proposal)
-        reviewed_commits = get_all_reviewed_commit_sha1(lp, proposal)
-        if latest_commit in reviewed_commits:
-            repo_logger.debug(
-                f"Skipping {proposal.web_link}: {latest_commit} has already been reviewed"
-            )
-            continue
-
-        # If WIP, see if this has been marked to be tested.
-        if proposal.queue_status == "Work in progress":
-            repo_logger.debug(f"Skipping {proposal.web_link}: WiP but no marker found")
-            continue
-        repo_logger.debug(f"Testing {proposal.web_link} !")
-        yield proposal
 
 
 def get_nice_repo_name(repo):