Block merge if graphite downstack is not yet merged

[TimonPost]

Checklist

• [x] I have read the Contributor Guide
• [x] I have read and agree to the Code of Conduct
• [x] I have added a description of my changes and why I'd like them included in the section below

Description of Changes

For graphite we want to have a protection mechanism that prevents stack branches from merging if downstack branches are not yet approved and merged.

I don't really know how to test this 'properly' or if it does what I want it to do. What would be a good way to test this before publishing life?

Some solutions

The tricky part is to check if downstack PR's are merged and only block merging in that case. Its tricky because we don't have access to the downstack PR's from within the code, as the code is based around analyzing the current PR.

Inspecting HTML

One possible approach is to inspect the graphites HTML comment code, assuming it remains relatively stable over time. By checking if the PR icon contains the 'open' class, we can determine whether the PR has been merged or not. This can be achieved by examining the following condition:

    async fn merge_blocked_by_graphite(&self) -> anyhow::Result<bool> {
        let pr_comments = self
            .client
            .get_pull_request_comments(self.config.name.as_str(), self.pr.number)
            .await?;

        for comment in pr_comments.iter().filter_map(|x| x.body_html) {
            if comment.contains("Current dependencies on/for this PR:") {
                let stack_pr_opened_status = comment
                    .lines()
                    .into_iter()
                    .filter(|x| x.contains("PR"))
                    .map(|x| x.contains("octicon-git-pull-request open color-fg-open"))
                    .collect::<Vec<_>>();

                let mut downstack_pr_opened = false;
                for current_opened in stack_pr_opened_status {
                    if downstack_pr_opened {
                        return Ok(true); // A downstack pr is opened, block till downstack branch is closed.
                    }
                    downstack_pr_opened = current_opened
                }                
            }
        }

        Ok(false)
    }

Two labels

An other approach is to add the block label only once to the Pull Request (PR). If the user removes the label, we should refrain from re-adding it to the PR. However, achieving this may not be straightforward. One potential solution is to introduce a second label that doesn't block the PR but serves as an indication that it is a "graphite" PR. We can add the block label only if the "graphite" label is not yet set. Consequently, if the user removes the block label and the "graphite" label is present, we won't add the block label again.

Iterating PR's

In the lib.rs file, we retrieve all the PRs from the repository. An alternative approach is to pass these PRs to the 'Analyzer', which is responsible for analyzing individual PRs. Within the 'Analyzer', we can extract the PR descriptions from the body of the Graphite's PR. By doing so, we can obtain a list of related PRs. From this list, we can then proceed to check the statuses of the stack PRs.

[repi]

I don't really know how to test this 'properly' or if it does what I want it to do. What would be a good way to test this before publishing life?

maybe we could set up an octobors-test github repo, a public one, that it runs on and we can do well testing on with new versions and with graphite and more? that could be quite nice to not risk our development production environments in other repos.

hopefully we can then point that repo to for example this PR to run the version of octobors from here, and do various test PRs, commits, merges, comments, etc there without making a mess of this or other repos while testing

[TimonPost]

I tested the code over in the new testing repo and running octobors locally with my github token. I went for the Two labels approach as that seems to only one that is possible without to much work. This means:

Add the block label only once to the Pull Request. If the user removes the label, we should refrain from re-adding it to the PR. We then also need to introduce a second label that doesn't block the PR but serves as an indication that it is a "graphite" PR. We can add the "block label" only if the "graphite" label is not yet set. Consequently, if the user removes the block label and the "graphite" label is present, we won't add the block label again.

Also made it so that the bot will react with a message on why it added the label. The bot can be pinged and will respond with more details.

Pr can be reviewed! Functionality works as expected.

[TimonPost]

ping: @repi @davidpdrsn

[repi]

not familiar with this codebase at all but will try to take a look.

CI is failing though on tests and cargo-deny, so need some cleanup & fixes most likely

[davidpdrsn]

What if the bot only merged PRs into main and not other branches? That feels simpler and might be good enough?

Then we wouldn’t need to detect if something is part of a stack.

[repi]

btw that is how I have configured Mergify in puffin repo, they have a config file we use where one can set which branches allow merging

[repi]

thought about it some more and that seems like it may be the easiest solution, only auto merge PRs that are targeted to main, instead of this change.

[TimonPost]

Sounds good, if we dont want auto-mege in custom branches, thus always manual merge for those, we can do it that way!

[davidpdrsn]

I'll close this for now but I definitely think the direction we previously discussed is the way to go!