launchpad-reviewers team mailing list archive

[Yuliy Schwartzburg <mp+486054@xxxxxxxxxxxxxxxxxx>]

Looks good, just a small comment about some weird logic I noticed about approval states.

Diff comments:

> diff --git a/lib/lp/code/model/branchmergeproposal.py b/lib/lp/code/model/branchmergeproposal.py
> index 0396b59..b6cd92b 100644
> --- a/lib/lp/code/model/branchmergeproposal.py
> +++ b/lib/lp/code/model/branchmergeproposal.py
> @@ -854,6 +864,192 @@ class BranchMergeProposal(StormBase, BugLinkTargetMixin):
>          # or superseded, then it is valid to be merged.
>          return self.queue_status not in FINAL_STATES
>  
> +    def isApproved(self):
> +        """See `IBranchMergeProposal`."""
> +
> +        # TODO ines-almeida 2025-05-09: the criteria for a merge proposal to
> +        # be 'approved' should eventually be defined by the repository owners.
> +        # For now we enforce that we need at least one approval from a trusted
> +        # reviewer, and no disapprovals (regardless of reviewer)
> +
> +        dissaproved_states = [
> +            CodeReviewVote.DISAPPROVE,
> +            CodeReviewVote.NEEDS_FIXING,
> +            CodeReviewVote.NEEDS_INFO,
> +            CodeReviewVote.NEEDS_RESUBMITTING,
> +        ]

We are only looking for one APPROVE, so I'm not sure why this is necessary. Is it just related to the todo? Like we can just collect all the `vote.comment.vote` of all trusted reviewers and check if CodeReviewVote.APPROVE is in that collection. We don't need 'disapproved_states in the current logic.

> +        approved = False
> +        for vote in self.votes:
> +            if not vote.comment:
> +                continue
> +            elif vote.comment.vote in dissaproved_states:
> +                return False
> +            elif (
> +                vote.comment.vote == CodeReviewVote.APPROVE
> +                and self.merge_target.isPersonTrustedReviewer(vote.reviewer)
> +            ):
> +                approved = True

you can break here if a True is found

> +        return approved
> +
> +    def CIChecksPassed(self):
> +        """See `IBranchMergeProposal`."""
> +
> +        # We don't run CI checks on bazaar repos
> +        if self.source_branch is not None:
> +            return True
> +
> +        reports = self.getStatusReports(self.source_git_commit_sha1)
> +        if reports.is_empty():
> +            return True
> +
> +        return reports.last().result == RevisionStatusResult.SUCCEEDED
> +
> +    def hasNoConflicts(self):
> +        """See `IBranchMergeProposal`."""
> +
> +        if self.preview_diff is None:
> +            return True
> +
> +        return (
> +            self.preview_diff.conflicts is None
> +            or self.preview_diff.conflicts == ""
> +        )
> +
> +    def hasNoPendingPrerequisite(self):
> +        """See `IBranchMergeProposal`."""
> +
> +        if self.merge_prerequisite is None:
> +            return True
> +
> +        if self.target_branch:
> +            raise NotImplementedError("Bazaar branches are not supported")
> +
> +        # Check if prerequisite has been merged into the target branch
> +        # The response returns a list of commits that were merged, meaning
> +        # that if the list is empty, the merge hasn't occured
> +        hosting_client = getUtility(IGitHostingClient)
> +        response = hosting_client.detectMerges(
> +            self.target_git_repository_id,
> +            self.target_git_commit_sha1,
> +            [self.prerequisite_git_commit_sha1],
> +        )
> +        return bool(response)
> +
> +    def diffIsUpToDate(self):
> +        """See `IBranchMergeProposal`."""
> +
> +        return self.next_preview_diff_job is None
> +
> +    def checkMergeCriteria(self, force=False):
> +        """See `IBranchMergeProposal`."""
> +
> +        # TODO ines-almeida 2025-05-05: the mergeability criteria should be
> +        # to some extent defined by the repository owner. While that
> +        # capability is not in place, we will set a few hard rules for one
> +        # to merge a MP from Launchpad
> +
> +        # List of hard rule criteria (defined by Launchpad)
> +        criteria = [
> +            (self.isInProgress, "Invalid status for merging"),
> +            (self.hasNoConflicts, "Diff contains conflicts"),
> +            (self.hasNoPendingPrerequisite, "Prerequisit branch not merged"),
> +            (self.diffIsUpToDate, "New changes were pushed too recently"),
> +        ]
> +
> +        # Criteria defined by project owners (skipped if 'force' merge)
> +        if not force:
> +            criteria.extend(
> +                [
> +                    (self.isApproved, "Proposal has not been approved"),
> +                    (self.CIChecksPassed, "CI checks failed"),
> +                ]
> +            )
> +
> +        result = True
> +        failed_checks = []
> +        for check, error_message in criteria:
> +            if not check():
> +                failed_checks.append(error_message)
> +                result = False
> +
> +        return result, failed_checks
> +
> +    def personCanMerge(self, person):
> +        """See `IBranchMergeProposal`."""
> +
> +        if self.source_branch is not None:
> +            raise NotImplementedError(
> +                "Merging Bazaar branches is not supported"
> +            )
> +
> +        permissions = self.target_git_repository.checkRefPermissions(
> +            person, [self.target_git_path]
> +        )
> +        return GitPermissionType.CAN_PUSH in permissions[self.target_git_path]
> +
> +    def merge(self, person, commit_message=None, force=False):
> +        """See `IBranchMergeProposal`."""
> +
> +        if not getFeatureFlag(PROPOSAL_MERGE_ENABLED_FEATURE_FLAG):
> +            raise BranchMergeProposalFeatureDisabled(
> +                "Merge API is currently disabled"
> +            )
> +
> +        if self.source_branch is not None:
> +            raise NotImplementedError(
> +                "Merging Bazaar branches is not supported"
> +            )
> +
> +        if not self.personCanMerge(person):
> +            raise Unauthorized()
> +
> +        can_be_merged, failed_checks = self.checkMergeCriteria(force)
> +        if not can_be_merged:
> +            raise BranchMergeProposalNotMergeable(
> +                "Merge proposal cannot be merged in its current state: %s"
> +                % ", ".join(failed_checks)
> +            )
> +
> +        if commit_message is None:
> +            commit_message = self.commit_message
> +
> +        hosting_client = getUtility(IGitHostingClient)
> +        response = hosting_client.merge(
> +            self.target_git_repository_id,
> +            self.target_git_ref.name,
> +            self.target_git_commit_sha1,
> +            self.source_git_ref.name,
> +            self.source_git_commit_sha1,
> +            person,
> +            commit_message,
> +            source_repo=self.source_git_repository_id,
> +        )
> +
> +        # It shouldn't be possible for the `merge_commit` to not exist in the
> +        # response given the request didn't raise, but we do a check anyways.
> +        if "merge_commit" not in response:
> +            raise BranchMergeProposalMergeFailed(
> +                "Failed to merge the proposal."
> +            )
> +
> +        merge_commit = response["merge_commit"]
> +        # Edge case, if `merge_commit` is None it means the merge proposal had
> +        # already been merged by the time this endpoint was run. Might happen
> +        # in the odd case that someone changes the status of a proposal from
> +        # 'merged' back to another state and then tries merging again.
> +        if merge_commit is None:
> +            return
> +
> +        # Update commit message in DB
> +        self.commit_message = commit_message
> +
> +        self.markAsMerged(
> +            merge_reporter=person,
> +            merged_revision_id=merge_commit,
> +            date_merged=UTC_NOW,
> +            merge_type=MergeType.REGULAR_MERGE,
> +        )
> +
>      def _reviewProposal(
>          self, reviewer, next_state, revision_id, _date_reviewed=None
>      ):


-- 
https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/486054
Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/launchpad:merge-button/add-merge-functionality into launchpad:master.