Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature/Idea]: cleanup - get all associated pull requests within one request #778

Closed
stevenlele opened this issue Aug 23, 2024 · 1 comment
Assignees

Comments

@stevenlele
Copy link

stevenlele commented Aug 23, 2024

What would you like to see changed/added?

Currently, cleanup command sends one API request per branch:

let pull_requests = stream::iter(branches.iter())
.map(|branch| async {
pb.inc(1);
if let Ok(Some(pull_request)) = github
.get_pull_request_from_branch(&default_branch, &branch.name)
.await
{
Some((pull_request, branch.name.as_str()))
} else {
None
}
})

Actually you can get the information within the get-branches request:

// Get all winget-pkgs branches from the user's fork except the default one
let (branches, repository_id, default_branch) =
github.get_branches(&github.get_username().await?).await?;

Here's an example of the GraphQL query:

 query GetBranches($owner: String!, $name: String!) {
   repository(name: $name, owner: $owner) {
     id
     defaultBranchRef {
       name
       id
     }
     refs(first: 100, refPrefix: "refs/heads/") {
       nodes {
         name
+        associatedPullRequests(
+          first: 5,  # fetch some more in case user opened PR against their own repo, or opened a new PR because previous one is closed
+        ) {
+          nodes {
+            title
+            url
+            state
+            repository {
+              nameWithOwner  # filter this by "microsoft/winget-pkgs"
+            }
+          }
+        }
       }
     }
   }
 }

You can pass the wanted states as a variable (as long as people don't re-open PRs with the same branch), or just check the state property in the result like how it's currently done. Edit: You should keep the current behavior. Filtering it in the query might cause false positives when the user opens a new PR when the previous one is closed unmerged.

To be safe, you should also fetch more than 1 PR per branch and filter repository.name_with_owner by microsoft/winget-pkgs. I sometimes mistakenly open pull requests again my own fork repo, especially when it's made through the web UI, and I think other people sometimes do this too. If we use first: 1, we might never get the correct PR against the microsoft repo. (Edit: it might make sense to make sure PRs against other repos are closed/merged too)

In fact this also fixes a bug caused by fetching PR by branch name instead of branch ID. I once reused a deleted branch name when manually creating a PR to modify a manifest I created, but Komac thinks this branch is merged and listed it in cleanup. By fetching associatedPullRequests, this issue is resolved automatically.

@russellbanks
Copy link
Owner

This was a brilliant issue, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants