launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #32762
[Merge] ~ines-almeida/launchpad:merge-button/add-request-merge-endpoint into launchpad:master
Ines Almeida has proposed merging ~ines-almeida/launchpad:merge-button/add-request-merge-endpoint into launchpad:master.
Commit message:
Add BranchMergeProposal.request_merge endpoint
This endpoint will replace the currently used merge() endpoint. The new request_merge calls turnip's request-merge API which triggers an async merge. This will prevent timeout issues when requesting a merge for large repositories
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/489106
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/launchpad:merge-button/add-request-merge-endpoint into launchpad:master.
diff --git a/lib/lp/code/interfaces/branchmergeproposal.py b/lib/lp/code/interfaces/branchmergeproposal.py
index 6226255..43079e3 100644
--- a/lib/lp/code/interfaces/branchmergeproposal.py
+++ b/lib/lp/code/interfaces/branchmergeproposal.py
@@ -906,9 +906,35 @@ class IBranchMergeProposalEdit(Interface):
@call_with(person=REQUEST_USER)
@export_write_operation()
@operation_for_version("devel")
- def merge(person, commit_message=None, force=False):
+ def request_merge(person, commit_message=None, force=False):
"""Request to merge this proposal.
+ This will make a request to the git hosting client to trigger a merge.
+ A successfull response to this endpoint does not necessarily mean the
+ merge was successfull, but rather that it was successfully requested.
+
+ :param person: The person requesting the merge.
+ :param commit_message: Allows overriding the commit message. If empty,
+ the existing commit_message is used (if any).
+ :param force: Whether to skip acceptance criteria.
+
+ :raises NotImplementedError: If using Bazaar branches.
+ :raises Unauthorized: If the person doesn't have permission to merge.
+ :raises BranchMergeProposalNotMergeable: If the proposal doesn't meet
+ all merge criteria.
+ :raises BranchMergeProposalMergeFailed: If the merge fails to be queued
+ """
+
+ @operation_parameters(
+ commit_message=Text(title="Override commit message"),
+ force=Bool(title="Whether to skip checks"),
+ )
+ @call_with(person=REQUEST_USER)
+ @export_write_operation()
+ @operation_for_version("devel")
+ def merge(person, commit_message=None, force=False):
+ """To be deprecated in favour of `request_merge`.
+
:param person: The person requesting the merge.
:param commit_message: Allows overriding the commit message. If empty,
the existing commit_message is used (if any).
diff --git a/lib/lp/code/interfaces/githosting.py b/lib/lp/code/interfaces/githosting.py
index a1d20db..4c008cb 100644
--- a/lib/lp/code/interfaces/githosting.py
+++ b/lib/lp/code/interfaces/githosting.py
@@ -170,7 +170,7 @@ class IGitHostingClient(Interface):
:param logger: An optional logger.
"""
- def merge(
+ def request_merge(
repo,
target_branch,
target_commit_sha1,
@@ -194,3 +194,28 @@ class IGitHostingClient(Interface):
service (if any)
:param logger: An optional logger.
"""
+
+ def merge(
+ repo,
+ target_branch,
+ target_commit_sha1,
+ source_branch,
+ source_commit_sha1,
+ commiter,
+ commit_message,
+ source_repo=None,
+ logger=None,
+ ):
+ """To be deprecated in favour of request_merge
+
+ :param repo: name of the target repository on the hosting service.
+ :param target_branch: name of target branch.
+ :param target_commit_sha1: commit set as target in target branch
+ :param source_branch: name of source branch, to be merged into target
+ :param source_commit_sha1: latest commit in source branch
+ :param committer: person that requests the merge
+ :param commit_message: custom message for merge commit
+ :param source_repo: name of the source repository on the hosting
+ service (if any)
+ :param logger: An optional logger.
+ """
diff --git a/lib/lp/code/model/branchmergeproposal.py b/lib/lp/code/model/branchmergeproposal.py
index 179ee62..bf14b74 100644
--- a/lib/lp/code/model/branchmergeproposal.py
+++ b/lib/lp/code/model/branchmergeproposal.py
@@ -1027,7 +1027,7 @@ class BranchMergeProposal(StormBase, BugLinkTargetMixin):
)
return GitPermissionType.CAN_PUSH in permissions[self.target_git_path]
- def merge(self, person, commit_message=None, force=False):
+ def request_merge(self, person, commit_message=None, force=False):
"""See `IBranchMergeProposal`."""
if not getFeatureFlag(PROPOSAL_MERGE_ENABLED_FEATURE_FLAG):
@@ -1059,6 +1059,76 @@ class BranchMergeProposal(StormBase, BugLinkTargetMixin):
commit_message = self.commit_message
hosting_client = getUtility(IGitHostingClient)
+ response = hosting_client.request_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,
+ )
+
+ # Expected response when merge is queued:
+ # {"queued": True, "already_merged": False}
+ # If the merge had previously been merged, then expected response is:
+ # {"queued": False, "already_merged": True}
+
+ if "queued" not in response:
+ raise BranchMergeProposalMergeFailed(
+ "Request to merge this proposal failed."
+ )
+
+ if response["queued"]:
+ return "Merge successfully queued"
+
+ if not response["already_merged"]:
+ raise BranchMergeProposalMergeFailed(
+ "Merge proposal could not be queued for merging."
+ )
+
+ # If we reach here, then the merge had already been previously merged
+ # successfully, but Launchpad didn't mark the proposal as merged.
+ # Force a rescan in that case.
+ self.target_git_repository.rescan()
+ return "Proposal already merged, waiting for rescan"
+
+ def merge(self, person, commit_message=None, force=False):
+ """See `IBranchMergeProposal`.
+ TODO ines-almeida 2025-07-11: we want to remove this method in favor
+ of the request_merge() one. Keeping it for now while we migrate.
+ """
+
+ 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, criteria = self.checkMergeCriteria(force)
+ if not can_be_merged:
+ failed_checks = [
+ item["error"]
+ for item in criteria.values()
+ if item["passed"] is False
+ ]
+ 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,
diff --git a/lib/lp/code/model/githosting.py b/lib/lp/code/model/githosting.py
index cc5f84b..9ee567e 100644
--- a/lib/lp/code/model/githosting.py
+++ b/lib/lp/code/model/githosting.py
@@ -262,6 +262,56 @@ class GitHostingClient:
"Failed to get merge diff from Git repository: %s" % str(e)
)
+ def request_merge(
+ self,
+ repo,
+ target_branch,
+ target_commit_sha1,
+ source_branch,
+ source_commit_sha1,
+ commiter,
+ commit_message,
+ source_repo=None,
+ logger=None,
+ ):
+ """See `IGitHostingClient`."""
+
+ if logger is not None:
+ logger.info(
+ "Requesting merge for %s from %s to %s" % repo,
+ quote(source_branch),
+ quote(target_branch),
+ )
+
+ if source_repo:
+ repo = f"{repo}:{source_repo}"
+
+ url = "/repo/%s/request-merge/%s:%s" % (
+ repo,
+ quote(target_branch),
+ quote(source_branch),
+ )
+
+ json_data = {
+ "committer_name": commiter.display_name,
+ "committer_email": commiter.preferredemail.email,
+ "commit_message": commit_message,
+ "target_commit_sha1": target_commit_sha1,
+ "source_commit_sha1": source_commit_sha1,
+ }
+
+ if logger is not None:
+ logger.info("Sending request to turnip '%s'" % url)
+
+ try:
+ return self._post(url, json=json_data)
+ except requests.RequestException as e:
+ if e.response is not None and e.response.status_code == 404:
+ raise GitRepositoryScanFault("Repository or branch not found")
+ raise GitRepositoryScanFault(
+ "Failed to merge from Git repository: %s" % str(e)
+ )
+
def merge(
self,
repo,
diff --git a/lib/lp/code/model/tests/test_branchmergeproposal.py b/lib/lp/code/model/tests/test_branchmergeproposal.py
index abef332..7ebb7b1 100644
--- a/lib/lp/code/model/tests/test_branchmergeproposal.py
+++ b/lib/lp/code/model/tests/test_branchmergeproposal.py
@@ -4383,4 +4383,188 @@ class TestBranchMergeProposalMerge(TestCaseWithFactory):
self.assertEqual(MergeType.REGULAR_MERGE, self.proposal.merge_type)
+class TestBranchMergeProposalRequestMerge(TestCaseWithFactory):
+ """Test the request_merge method of BranchMergeProposal."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super().setUp()
+ self.hosting_fixture = self.useFixture(GitHostingFixture())
+ self.useFixture(
+ FeatureFixture({PROPOSAL_MERGE_ENABLED_FEATURE_FLAG: "on"})
+ )
+
+ self.proposal = removeSecurityProxy(
+ self.factory.makeBranchMergeProposalForGit()
+ )
+
+ self.person = self.factory.makePerson()
+ self.reviewer = self.proposal.target_git_repository.owner
+
+ rule = removeSecurityProxy(
+ self.factory.makeGitRule(
+ repository=self.proposal.target_git_repository
+ )
+ )
+ self.factory.makeGitRuleGrant(
+ rule=rule, grantee=self.person, can_create=True, can_push=True
+ )
+
+ def test_request_merge_feature_flag(self):
+ self.useFixture(
+ FeatureFixture({PROPOSAL_MERGE_ENABLED_FEATURE_FLAG: ""})
+ )
+ self.assertRaises(
+ BranchMergeProposalFeatureDisabled,
+ self.proposal.request_merge,
+ self.person,
+ )
+
+ def test_request_merge_success(self):
+ repository = self.proposal.target_git_repository
+ [source_ref, target_ref] = self.factory.makeGitRefs(
+ paths=["refs/heads/source", "refs/heads/target"],
+ repository=repository,
+ )
+ proposal = removeSecurityProxy(
+ self.factory.makeBranchMergeProposalForGit(
+ source_ref=source_ref,
+ target_ref=target_ref,
+ )
+ )
+ proposal.createComment(
+ owner=self.reviewer,
+ vote=CodeReviewVote.APPROVE,
+ )
+ proposal.next_preview_diff_job.start()
+ proposal.next_preview_diff_job.complete()
+ with person_logged_in(self.person):
+ result = proposal.request_merge(self.person)
+ self.assertEqual("Merge successfully queued", result)
+
+ def test_request_cross_repo_merge_success(self):
+ self.proposal.createComment(
+ owner=self.reviewer,
+ vote=CodeReviewVote.APPROVE,
+ )
+ self.proposal.next_preview_diff_job.start()
+ self.proposal.next_preview_diff_job.complete()
+ with person_logged_in(self.person):
+ result = self.proposal.request_merge(self.person)
+ self.assertEqual("Merge successfully queued", result)
+
+ def test_request_force_merge(self):
+ self.proposal.next_preview_diff_job.start()
+ self.proposal.next_preview_diff_job.complete()
+ with person_logged_in(self.person):
+ self.assertRaises(
+ BranchMergeProposalNotMergeable,
+ self.proposal.request_merge,
+ self.person,
+ )
+ result = self.proposal.request_merge(self.person, force=True)
+ self.assertEqual("Merge successfully queued", result)
+
+ def test_request_merge_success_commit_message(self):
+ self.proposal.createComment(
+ owner=self.reviewer,
+ vote=CodeReviewVote.APPROVE,
+ )
+ self.proposal.next_preview_diff_job.start()
+ self.proposal.next_preview_diff_job.complete()
+ self.proposal.commit_message = "Old commit message"
+ with person_logged_in(self.person):
+ result = self.proposal.request_merge(
+ self.person,
+ commit_message="New commit message",
+ )
+ self.assertEqual("Merge successfully queued", result)
+ self.proposal = Store.of(self.proposal).get(
+ BranchMergeProposal, self.proposal.id
+ )
+ self.assertEqual("Old commit message", self.proposal.commit_message)
+
+ def test_request_merge_unsuccessful_commit_message(self):
+ self.proposal.createComment(
+ owner=self.reviewer,
+ vote=CodeReviewVote.APPROVE,
+ )
+ self.proposal.next_preview_diff_job.start()
+ self.proposal.next_preview_diff_job.complete()
+ self.proposal.commit_message = "Old commit message"
+ self.hosting_fixture.request_merge.failure = (
+ BranchMergeProposalMergeFailed("Merge proposal failed to merge")
+ )
+ with person_logged_in(self.person):
+ self.assertRaises(
+ BranchMergeProposalMergeFailed,
+ self.proposal.request_merge,
+ self.person,
+ commit_message="New commit message",
+ )
+ self.proposal = Store.of(self.proposal).get(
+ BranchMergeProposal, self.proposal.id
+ )
+ self.assertEqual(self.proposal.commit_message, "Old commit message")
+
+ def test_request_merge_no_permission(self):
+ person = self.factory.makePerson()
+ self.assertRaises(
+ Unauthorized,
+ self.proposal.request_merge,
+ person,
+ )
+
+ def test_request_merge_not_mergeable(self):
+ self.assertRaises(
+ BranchMergeProposalNotMergeable,
+ self.proposal.request_merge,
+ self.person,
+ )
+
+ def test_request_merge_bazaar_not_supported(self):
+ proposal = removeSecurityProxy(self.factory.makeBranchMergeProposal())
+ self.assertRaises(
+ NotImplementedError,
+ proposal.request_merge,
+ self.person,
+ )
+
+ def test_request_merge_turnip_failure(self):
+ self.proposal.createComment(
+ owner=self.reviewer,
+ vote=CodeReviewVote.APPROVE,
+ )
+ self.proposal.next_preview_diff_job.start()
+ self.proposal.next_preview_diff_job.complete()
+ self.hosting_fixture.request_merge.failure = (
+ BranchMergeProposalMergeFailed("Merge proposal failed to merge")
+ )
+ with person_logged_in(self.person):
+ self.assertRaises(
+ BranchMergeProposalMergeFailed,
+ self.proposal.request_merge,
+ self.person,
+ )
+
+ def test_request_merge_already_merged(self):
+ self.proposal.createComment(
+ owner=self.reviewer,
+ vote=CodeReviewVote.APPROVE,
+ )
+ self.proposal.next_preview_diff_job.start()
+ self.proposal.next_preview_diff_job.complete()
+ self.hosting_fixture.request_merge.result = {
+ "queued": False,
+ "already_merged": True,
+ }
+ with person_logged_in(self.person):
+ result = self.proposal.request_merge(self.person)
+ self.assertEqual(
+ "Proposal already merged, waiting for rescan",
+ result,
+ )
+
+
load_tests = load_tests_apply_scenarios
diff --git a/lib/lp/code/model/tests/test_githosting.py b/lib/lp/code/model/tests/test_githosting.py
index 08fdf2c..3fef71b 100644
--- a/lib/lp/code/model/tests/test_githosting.py
+++ b/lib/lp/code/model/tests/test_githosting.py
@@ -834,3 +834,116 @@ class TestGitHostingClientMerge(TestCaseWithFactory):
person,
"Test commit message",
)
+
+
+class TestGitHostingClientRequestMerge(TestCaseWithFactory):
+ """Test the request-merge method of GitHostingClient."""
+
+ layer = ZopelessDatabaseLayer
+
+ def setUp(self):
+ super().setUp()
+ self.client = getUtility(IGitHostingClient)
+ self.requests = []
+
+ @contextmanager
+ def mockRequests(self, method, set_default_timeout=True, **kwargs):
+ with responses.RequestsMock() as requests_mock:
+ requests_mock.add(method, re.compile(r".*"), **kwargs)
+ original_timeout_function = get_default_timeout_function()
+ if set_default_timeout:
+ set_default_timeout_function(lambda: 60.0)
+ try:
+ yield
+ finally:
+ set_default_timeout_function(original_timeout_function)
+ self.requests = [call.request for call in requests_mock.calls]
+
+ def test_request_merge_success(self):
+ """Test successful request_merge"""
+ person = self.factory.makePerson()
+
+ with self.mockRequests(
+ "POST", json={"queued": True, "already_merged": False}
+ ):
+ response = self.client.request_merge(
+ 1, # repository id
+ "target-branch",
+ "target_commit_sha1",
+ "source-branch",
+ "source_commit_sha1",
+ person,
+ "Test commit message",
+ )
+
+ self.assertTrue(response["queued"])
+ self.assertEqual(
+ "http://git.launchpad.test:19417/repo/1/request-merge/";
+ "target-branch:source-branch",
+ self.requests[0].url,
+ )
+
+ def test_request_cross_repo_merge_success(self):
+ """Test successful request_merge for cross repo branches"""
+ person = self.factory.makePerson()
+
+ with self.mockRequests(
+ "POST", json={"queued": True, "already_merged": False}
+ ):
+ response = self.client.request_merge(
+ 1, # repository id
+ "target-branch",
+ "target_commit_sha1",
+ "source-branch",
+ "source_commit_sha1",
+ person,
+ "Test commit message",
+ source_repo=2, # source repository id
+ )
+
+ self.assertTrue(response["queued"])
+ self.assertEqual(
+ "http://git.launchpad.test:19417/repo/1:2/request-merge/";
+ "target-branch:source-branch",
+ self.requests[0].url,
+ )
+
+ def test_request_merge_not_found(self):
+ """Test request_merge with non-existent repository"""
+
+ person = self.factory.makePerson()
+ with self.mockRequests("POST", status=404):
+ self.assertRaises(
+ GitRepositoryScanFault,
+ self.client.request_merge,
+ "non-existent",
+ "target-branch",
+ "target_commit_sha1",
+ "source-branch",
+ "source_commit_sha1",
+ person,
+ "Test commit message",
+ )
+
+ def test_request_merge_network_error(self):
+ """Test request_merge with network error"""
+ self.hosting_fixture = self.useFixture(GitHostingFixture())
+ self.hosting_fixture.request_merge.failure = GitRepositoryScanFault(
+ "Network error"
+ )
+ self.client = getUtility(IGitHostingClient)
+
+ repository = self.factory.makeGitRepository()
+ person = self.factory.makePerson()
+
+ self.assertRaises(
+ GitRepositoryScanFault,
+ self.client.request_merge,
+ repository.id,
+ "target-branch",
+ "target_commit_sha1",
+ "source-branch",
+ "source_commit_sha1",
+ person,
+ "Test commit message",
+ )
diff --git a/lib/lp/code/tests/helpers.py b/lib/lp/code/tests/helpers.py
index 7ed3044..ecda57d 100644
--- a/lib/lp/code/tests/helpers.py
+++ b/lib/lp/code/tests/helpers.py
@@ -383,6 +383,9 @@ class GitHostingFixture(fixtures.Fixture):
self.detectMerges = fake_method_factory(
result=({} if merges is None else merges)
)
+ self.request_merge = fake_method_factory(
+ result=({"queued": True, "already_merged": False})
+ )
self.merge = fake_method_factory(
result=({"merge_commit": "fake-sha1", "previously_merged": False})
)
