launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #32814
[Merge] ~ines-almeida/launchpad:merge-button/improve-criteria-checking into launchpad:master
Ines Almeida has proposed merging ~ines-almeida/launchpad:merge-button/improve-criteria-checking into launchpad:master.
Commit message:
Update merge criteria check functions
It now returns whether the criteria is required or not, and whether a proposal is force mergeable
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/490022
Small update that will allow less hardcoded code in the UI, and updates which criteria is mandatory (now only conflicts is a real blocker for merging)
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/launchpad:merge-button/improve-criteria-checking into launchpad:master.
diff --git a/lib/lp/code/interfaces/branchmergeproposal.py b/lib/lp/code/interfaces/branchmergeproposal.py
index 43079e3..3db350d 100644
--- a/lib/lp/code/interfaces/branchmergeproposal.py
+++ b/lib/lp/code/interfaces/branchmergeproposal.py
@@ -636,7 +636,7 @@ class IBranchMergeProposalView(Interface):
'criteria' key contains details about each check."
"""
- def checkMergeCriteria(force=False):
+ def checkMergeCriteria():
"""Check if the merge proposal meets all criteria for merging.
The hard criteria are:
@@ -649,8 +649,6 @@ class IBranchMergeProposalView(Interface):
- Has at least one approval from a trusted reviewer
- CI checks have passed
- :param force: Whether to skip approval and CI checks
-
:return: A tuple of (bool, list) where the bool indicates if all
criteria are met, and the list contains failed criteria.
"""
diff --git a/lib/lp/code/model/branchmergeproposal.py b/lib/lp/code/model/branchmergeproposal.py
index bf14b74..da5d6cf 100644
--- a/lib/lp/code/model/branchmergeproposal.py
+++ b/lib/lp/code/model/branchmergeproposal.py
@@ -934,14 +934,17 @@ class BranchMergeProposal(StormBase, BugLinkTargetMixin):
def getMergeCriteria(self):
"""See `IBranchMergeProposal`."""
- can_be_merged, criteria = self.checkMergeCriteria()
+ can_be_merged, can_be_force_merged, criteria = (
+ self.checkMergeCriteria()
+ )
return {
"can_be_merged": can_be_merged,
+ "can_be_force_merged": can_be_force_merged,
"criteria": criteria,
}
- def checkMergeCriteria(self, force=False):
+ def checkMergeCriteria(self):
"""See `IBranchMergeProposal`."""
# TODO ines-almeida 2025-05-05: the mergeability criteria should be
@@ -954,61 +957,53 @@ class BranchMergeProposal(StormBase, BugLinkTargetMixin):
MergeProposalCriteria.IS_IN_PROGRESS: (
self.isInProgress,
"Invalid status for merging",
+ False,
),
MergeProposalCriteria.HAS_NO_CONFLICTS: (
self.hasNoConflicts,
"Diff contains conflicts",
+ True,
),
MergeProposalCriteria.DIFF_IS_UP_TO_DATE: (
self.diffIsUpToDate,
"New changes were pushed too recently",
+ False,
),
MergeProposalCriteria.HAS_NO_PENDING_PREREQUISITE: (
self.hasNoPendingPrerequisite,
"Prerequisite branch not merged",
+ False,
),
MergeProposalCriteria.IS_APPROVED: (
self.isApproved,
"Proposal has not been approved",
+ False,
),
MergeProposalCriteria.CI_CHECKS_PASSED: (
self.CIChecksPassed,
"CI checks failed",
+ False,
),
}
- criteria_to_run = [
- MergeProposalCriteria.IS_IN_PROGRESS,
- MergeProposalCriteria.HAS_NO_CONFLICTS,
- MergeProposalCriteria.DIFF_IS_UP_TO_DATE,
- ]
-
- # This is only a criteria if there is a prerequisit branch
- if self.merge_prerequisite is not None:
- criteria_to_run.append(
- MergeProposalCriteria.HAS_NO_PENDING_PREREQUISITE
- )
-
- # Criteria defined by project owners (skipped if 'force' merge)
- if not force:
- criteria_to_run.append(MergeProposalCriteria.IS_APPROVED)
- criteria_to_run.append(MergeProposalCriteria.CI_CHECKS_PASSED)
-
- result = True
+ can_be_merged = True
+ can_be_force_merged = True
status = {}
- for criteria in criteria_to_run:
- check, error = MERGE_CRITERIA[criteria]
+ for key, criteria in MERGE_CRITERIA.items():
+ check, error, is_required = criteria
passed = check()
- key = str(criteria)
- status[key] = {"passed": passed}
+ key = str(key)
+ status[key] = {"passed": passed, "is_required": is_required}
if passed is None:
status[key]["error"] = "Not implemented"
elif not passed:
status[key]["error"] = error
- result = False
+ can_be_merged = False
+ if is_required:
+ can_be_force_merged = False
- return result, status
+ return can_be_merged, can_be_force_merged, status
def canIMerge(self, person):
"""See `IBranchMergeProposal`."""
@@ -1043,8 +1038,10 @@ class BranchMergeProposal(StormBase, BugLinkTargetMixin):
if not self.personCanMerge(person):
raise Unauthorized()
- can_be_merged, criteria = self.checkMergeCriteria(force)
- if not can_be_merged:
+ can_be_merged, can_be_force_merged, criteria = (
+ self.checkMergeCriteria()
+ )
+ if not can_be_merged and not (force and can_be_force_merged):
failed_checks = [
item["error"]
for item in criteria.values()
@@ -1113,8 +1110,10 @@ class BranchMergeProposal(StormBase, BugLinkTargetMixin):
if not self.personCanMerge(person):
raise Unauthorized()
- can_be_merged, criteria = self.checkMergeCriteria(force)
- if not can_be_merged:
+ can_be_merged, can_be_force_merged, criteria = (
+ self.checkMergeCriteria()
+ )
+ if not can_be_merged and not (force and can_be_force_merged):
failed_checks = [
item["error"]
for item in criteria.values()
diff --git a/lib/lp/code/model/tests/test_branchmergeproposal.py b/lib/lp/code/model/tests/test_branchmergeproposal.py
index 7ebb7b1..965364c 100644
--- a/lib/lp/code/model/tests/test_branchmergeproposal.py
+++ b/lib/lp/code/model/tests/test_branchmergeproposal.py
@@ -3854,11 +3854,15 @@ class TestBranchMergeProposalMergeCriteria(
self.useFixture(GitHostingFixture())
self.expected_criteria = {
- "is_in_progress": {"passed": True},
- "has_no_conflicts": {"passed": True},
- "diff_is_up_to_date": {"passed": True},
- "is_approved": {"passed": True},
- "CI_checks_passed": {"passed": True},
+ "is_in_progress": {"passed": True, "is_required": False},
+ "has_no_conflicts": {"passed": True, "is_required": True},
+ "diff_is_up_to_date": {"passed": True, "is_required": False},
+ "is_approved": {"passed": True, "is_required": False},
+ "CI_checks_passed": {"passed": True, "is_required": False},
+ "has_no_pending_prerequisite": {
+ "passed": True,
+ "is_required": False,
+ },
}
def test_checkMergeCriteria_all_passed(self):
@@ -3871,46 +3875,30 @@ class TestBranchMergeProposalMergeCriteria(
)
self.proposal.next_preview_diff_job.start()
self.proposal.next_preview_diff_job.complete()
- can_merge, criteria = self.proposal.checkMergeCriteria()
+ can_merge, can_force, criteria = self.proposal.checkMergeCriteria()
self.assertTrue(can_merge)
+ self.assertTrue(can_force)
self.assertDictEqual(self.expected_criteria, criteria)
def test_checkMergeCriteria_some_failed(self):
# If some criteria are not met, checkMergeCriteria returns a tuple with
# False, and a dict stating which criteria failed.
- can_merge, criteria = self.proposal.checkMergeCriteria()
+ can_merge, can_force, criteria = self.proposal.checkMergeCriteria()
self.assertFalse(can_merge)
+ self.assertTrue(can_force)
expected_criteria = self.expected_criteria
expected_criteria["diff_is_up_to_date"] = {
"passed": False,
+ "is_required": False,
"error": "New changes were pushed too recently",
}
expected_criteria["is_approved"] = {
"passed": False,
+ "is_required": False,
"error": "Proposal has not been approved",
}
self.assertDictEqual(expected_criteria, criteria)
- def test_checkMergeCriteria_force(self):
- # Some criteria can be skippedf with force=True
-
- self.proposal.next_preview_diff_job.start()
- self.proposal.next_preview_diff_job.complete()
-
- # Return False if not using force=True
- can_merge, criteria = self.proposal.checkMergeCriteria()
- self.assertFalse(can_merge)
- expected_criteria = self.expected_criteria
- expected_criteria["is_approved"] = {
- "passed": False,
- "error": "Proposal has not been approved",
- }
- self.assertDictEqual(expected_criteria, criteria)
-
- # Return False if not using force=True
- can_merge, criteria = self.proposal.checkMergeCriteria(force=True)
- self.assertTrue(can_merge)
-
def test_checkMergeCriteria_with_prerequisite(self):
# When there is a prerequisite branch, its criteria is included
@@ -3934,18 +3922,23 @@ class TestBranchMergeProposalMergeCriteria(
expected_criteria = self.expected_criteria
if self.git:
expected_can_merge = False
+ expected_can_force = True
expected_criteria["has_no_pending_prerequisite"] = {
"passed": False,
+ "is_required": False,
"error": "Prerequisite branch not merged",
}
else:
expected_can_merge = True
+ expected_can_force = True
expected_criteria["has_no_pending_prerequisite"] = {
"passed": None,
+ "is_required": False,
"error": "Not implemented",
}
- can_merge, criteria = proposal.checkMergeCriteria()
+ can_merge, can_force, criteria = proposal.checkMergeCriteria()
self.assertEqual(expected_can_merge, can_merge)
+ self.assertEqual(expected_can_force, can_force)
self.assertDictEqual(expected_criteria, criteria)
def test_getMergeCriteria_initial_state(self):
@@ -3953,18 +3946,25 @@ class TestBranchMergeProposalMergeCriteria(
# the merge criteria statuses
expected_criteria = {
"can_be_merged": False,
+ "can_be_force_merged": True,
"criteria": {
- "is_in_progress": {"passed": True},
- "has_no_conflicts": {"passed": True},
+ "is_in_progress": {"passed": True, "is_required": False},
+ "has_no_conflicts": {"passed": True, "is_required": True},
"diff_is_up_to_date": {
"passed": False,
+ "is_required": False,
"error": "New changes were pushed too recently",
},
"is_approved": {
"passed": False,
+ "is_required": False,
"error": "Proposal has not been approved",
},
- "CI_checks_passed": {"passed": True},
+ "CI_checks_passed": {"passed": True, "is_required": False},
+ "has_no_pending_prerequisite": {
+ "passed": True,
+ "is_required": False,
+ },
},
}
@@ -3984,6 +3984,7 @@ class TestBranchMergeProposalMergeCriteria(
response = self.proposal.getMergeCriteria()
expected_response = {
"can_be_merged": True,
+ "can_be_force_merged": True,
"criteria": self.expected_criteria,
}
self.assertDictEqual(expected_response, response)
@@ -4001,28 +4002,33 @@ class TestBranchMergeProposalMergeCriteria(
expected_response = {
"can_be_merged": False,
+ "can_be_force_merged": True,
"criteria": {
- "is_in_progress": {"passed": True},
- "has_no_conflicts": {"passed": True},
+ "is_in_progress": {"passed": True, "is_required": False},
+ "has_no_conflicts": {"passed": True, "is_required": True},
"diff_is_up_to_date": {
"passed": False,
+ "is_required": False,
"error": "New changes were pushed too recently",
},
- "has_no_pending_prerequisite": {
- "passed": False,
- "error": "Prerequisite branch not merged",
- },
"is_approved": {
"passed": False,
+ "is_required": False,
"error": "Proposal has not been approved",
},
- "CI_checks_passed": {"passed": True},
+ "CI_checks_passed": {"passed": True, "is_required": False},
+ "has_no_pending_prerequisite": {
+ "passed": False,
+ "is_required": False,
+ "error": "Prerequisite branch not merged",
+ },
},
}
if not self.git:
expected_response["criteria"]["has_no_pending_prerequisite"] = {
"passed": None,
+ "is_required": False,
"error": "Not implemented",
}
