Merge ~lofidevops/ols-jenkaas:sn1780-mergebot-dependent-branches into ols-jenkaas:main

[David]

Add check for merged dependencies before landing a proposal. Fixes SN-1780.

All dependencies of a given proposal must be merged. If not, do not merge.

This will prevent developers accidentally merging proposals with pending dependencies.

[Emma Brostrom (emmabrostrom)]

Thanks for this David, looks great! I wanted to mention one thing - this approach is only inclusive of git repositories, and some of ours are bzr. I think you'll want to add some logic that takes into account bzr proposals. Here is an example of what that logic could look like: https://git.launchpad.net/tarmac/tree/tarmac/bin/commands.py#n357. I haven't thought too much about the implementation, but it could look like using the LandBzrProposal and LandGitProposal subclasses of LandApprovedProposals.

[Maximiliano Bertacchini (maxiberta)]

This is great!! But I don't really understand the matching logic, check below. Sorry!
Thanks

145457a... by David on 2024-04-03

Use Sphinx formatting.

Signed-off-by: David Seaward <email address hidden>

4986524... by David on 2024-04-03

Refactor dependency logic.

Signed-off-by: David Seaward <email address hidden>

[David (lofidevops)]

Responded.

[David (lofidevops)]

Just a comment to confirm that we decided not to support bzr.

4a4822c... by David on 2024-05-09

Fix selection logic for prerequisite proposals. Use generator rather than list.

Signed-off-by: David Seaward <email address hidden>

[Emma Brostrom (emmabrostrom)]

Thanks for the updates David! I had a few questions below. Also as far as testing goes - you could create your own repo, similar to what I did for pre-merge CI:https://code.launchpad.net/~ubuntuone-hackers/golem/+git/golem-tests. Or just use that one honestly. But you can use that to create some MPs with dependencies and test it out with lp-shell.

review: Needs Information

aaa94e1... by David on 2024-05-10

Fix prerequisite logic to use the correct fields. Add comment.

Signed-off-by: David Seaward <email address hidden>

[Emma Brostrom (emmabrostrom)]

Thanks David! A few nitpicks/ one question.

review: Needs Information

[Guillermo Gonzalez (verterok)]

one comment regarding getMergeProposals

[Emma Brostrom (emmabrostrom)]

Did a quick test using lp-shell - see comment below

[Guillermo Gonzalez (verterok)]

> Did a quick test using lp-shell - see comment below

I see 3 in https://code.launchpad.net/~ubuntuone-pqm-team/golem/+git/golem/+ref/main/+activereviews but one is a WIP one.

Could you check if the 3 you get in lp-shell have different status?

[Emma Brostrom (emmabrostrom)]

Updated paste: https://pastebin.canonical.com/p/x5VTxJsFYV/

Work in progress is considered an active review. The 3 MPs found from repo.getMergeProposals() are the ones from active review. I don't believe a "Merged" MP would be returned from this.

[Guillermo Gonzalez (verterok)]

> Updated paste: https://pastebin.canonical.com/p/x5VTxJsFYV/
>
> Work in progress is considered an active review. The 3 MPs found from
> repo.getMergeProposals() are the ones from active review. I don't believe a
> "Merged" MP would be returned from this.

Indeed, merged ones need to be explicitly requested.

Thanks for checking!

[Maximiliano Bertacchini (maxiberta)]

Looks great to me! +1 provided all relevant comments/suggestions are addressed/replied. Thanks

review: Approve

Unmerged commits

aaa94e1... by David on 2024-05-10

Fix prerequisite logic to use the correct fields. Add comment.

Signed-off-by: David Seaward <email address hidden>

4a4822c... by David on 2024-05-09

Fix selection logic for prerequisite proposals. Use generator rather than list.

Signed-off-by: David Seaward <email address hidden>

4986524... by David on 2024-04-03

Refactor dependency logic.

Signed-off-by: David Seaward <email address hidden>

145457a... by David on 2024-04-03

Use Sphinx formatting.

Signed-off-by: David Seaward <email address hidden>

8157455... by David on 2024-03-27

Add check for merged dependencies before landing a proposal.

Signed-off-by: David Seaward <email address hidden>

diff --git a/olsjenkaas/commands.py b/olsjenkaas/commands.py
index 971843a..b07563f 100644
--- a/olsjenkaas/commands.py
+++ b/olsjenkaas/commands.py
@@ -531,8 +531,13 @@ class LandApprovedProposal(CommandWithContainer):
 
     def land_proposal(self, proposal, target_url):
         """
-        Land a proposal (validate, merge, run landing tests, push upstream).
-        Returns 0 if successful, otherwise 1.
+        Lands a proposal by performing the necessary steps including validation,
+        merging, running tests, pushing merged branch, and updating the proposal status.
+
+        :param proposal: The proposal to land. See https://api.launchpad.net/devel.html#branch_merge_proposal
+            for data structure.
+        :param target_url: The URL of the merge target.
+        :return: 0 if the proposal was successfully landed, 1 if it failed or was rejected.
         """
 
         logger.info("Trying to land {}".format(proposal.web_link))
@@ -542,6 +547,8 @@ class LandApprovedProposal(CommandWithContainer):
                 raise Rejected("A commit message must be set.")
             if not launchpad.is_approved(proposal, self.target):
                 raise Rejected("Voting criteria not met.")
+            if not launchpad.all_dependencies_merged(proposal):
+                raise Rejected("A dependent proposal is still not merged.")
 
             # prepare merge target
             local_work_dir = os.path.realpath(self.options.work_dir)
diff --git a/olsjenkaas/launchpad.py b/olsjenkaas/launchpad.py
index 0f3d653..68f941e 100644
--- a/olsjenkaas/launchpad.py
+++ b/olsjenkaas/launchpad.py
@@ -104,3 +104,44 @@ def is_approved(proposal, target):
          votes['Needs Information'] == 0)):
         approved = True
     return approved
+
+
+def get_dependencies(proposal):
+    """
+    Get all dependencies of the proposal.
+
+    :param proposal: The proposal with dependencies to fetch. See https://api.launchpad.net/devel.html#branch_merge_proposal
+        for data structure.
+    :return: A list of all dependencies
+    """
+
+    if not proposal.prerequisite_git_repository:
+        return []
+
+    # search all merge proposals of the parent git repository
+    # find candidates where MP source (identity) matches the
+    # original proposal's prerequisite
+    prerequisites = (
+        mp
+        for mp in proposal.target_git_repository.getMergeProposals()
+        if mp.source_git_path
+        and mp.source_git_path == proposal.prerequisite_git_path
+    )
+
+    return prerequisites
+
+
+def all_dependencies_merged(proposal):
+    """
+    Checks if all dependencies of a proposal have been merged.
+
+    :param proposal: The proposal with dependencies to check. See https://api.launchpad.net/devel.html#branch_merge_proposal
+        for data structure.
+    :return: True if all dependencies are merged, False otherwise.
+    """
+
+    for dependency in get_dependencies(proposal):
+        if dependency.queue_status != 'Merged':
+            return False
+
+    return True