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

Determine if pr is ready to land #10

Closed
2 tasks done
diananova opened this issue Feb 15, 2021 · 11 comments
Closed
2 tasks done

Determine if pr is ready to land #10

diananova opened this issue Feb 15, 2021 · 11 comments

Comments

@diananova
Copy link

@ljharb
Copy link
Owner

ljharb commented Feb 15, 2021

We'll want to make sure this includes "required review" status, and that this reports non-required status checks (but does not consider their status towards the overarching success/failure state), and that if the PR has merge conflicts, it should not be considered ready to land.

I don't personally use codeowners, but ideally as well, if a codeowner review is still outstanding, it should not report as ready.

@sladyn98
Copy link

We could probably add functions to the package https://github.com/sladyn98/hawk-eye-npm that perform something like this:

  1. queryForRequiredReviews: Queries the PR for reviews and returns a boolean true/false depending on whether the PR has requested changes or is still pending reviews. I am not too sure how we would check for code owners but I guesswe can check if the PR has been approved by maintainers.

  2. queryForMergeConflicts: Pretty self-explanatory that checks the PR for merge conflicts and returns a boolean if it has a conflict or no

@sladyn98
Copy link

sladyn98 commented Feb 15, 2021

@sladyn98
Copy link

{
  "data": {
    "repository": {
      "url": "https://github.com/jenkinsci/custom-distribution-service",
      "pullRequest": {
        "number": 162,
        "url": "https://github.com/jenkinsci/custom-distribution-service/pull/162",
        "mergeable": "CONFLICTING",
        "reviewDecision": "CHANGES_REQUESTED",
        "author": {
          "login": "sladyn98",
          "url": "https://github.com/sladyn98"
        },
        "commits": {
          "nodes": [
            {
              "commit": {
                "commitUrl": "https://github.com/jenkinsci/custom-distribution-service/commit/4648520244d8879c60ddcb2b53b78c6164b4715b",
                "oid": "4648520244d8879c60ddcb2b53b78c6164b4715b",
                "status": {
                  "state": "FAILURE",
                  "contexts": [
                    {
                      "state": "ERROR",
                      "targetUrl": "https://ci.jenkins.io/job/Tools/job/custom-distribution-service/job/PR-162/7/display/redirect",
                      "description": "The build of this commit was aborted",
                      "context": "continuous-integration/jenkins/pr-merge"
                    }
                  ]
                }
              }
            }
          ]

@thehanimo
Copy link

thehanimo commented Feb 16, 2021

I feel that this shouldn't be a separate package, at least for now, since adding functions to a package, publishing it and then updating dependencies in our project doesn't seem ideal since we will have a ton of iterations.

I've merged the feature-list branch to main on the MLH fork. I was thinking we can open up a new feature-PR branch and use the code (or more specifically, just the GraphQL query) from sladyn98/hawk-eye-npm instead of the npm package to build on this feature.

Need your thoughts @ljharb @sladyn98 @diananova

@sladyn98
Copy link

@thehanimo Yeah makes sense, I agree with you. I was just playing around with the code and was looking at ways it could be made modular.

@ljharb
Copy link
Owner

ljharb commented Feb 16, 2021

Totally fine to keep in one repo for now; however, my ideal end state is the maximum number of packages at the minimal size/responsibility each :-)

@sladyn98
Copy link

b) hawk.isMergeable : Checks if a PR has any merge conflicts and whether it can be merged

hawk.isMergeable({
      token  : '',
      owner  : 'jenkinsci',
      repo   : 'custom-distribution-service',
      pr     : '162'
  }).then(res => console.log(res)) 

b) hawk.isPendingReview : Checks if a PR has pending reviews or is approved

Usage

hawk.isPendingReview({
      token  : '',
      owner  : 'jenkinsci',
      repo   : 'custom-distribution-service',
      pr     : '162'
  }).then(res => console.log(res))

Added some more options to the package that will tell us if the pr is mergeable or not and if it has pending review.
I am not too sure how would we integrate the polling feature. I mean that will continuously poll if a pr has been approved or no. I am thinking in two ways :

  • Maybe we could spin up a lamba function that we could hit after an interval and get the status of the PR
  • We could also run a cron job on an ec2 instance

However these two solutions would mean we would be taking care of the maintenance and running of these instances/servers and I am not quite sure if that is the right way to go.

What do you guys think @thehanimo @ljharb @diananova

@ljharb
Copy link
Owner

ljharb commented Feb 17, 2021

I was assuming polling would be achieved just by looping on the client.

@sladyn98
Copy link

Yeah that could be done as well :) Simple and efficient

@ljharb ljharb transferred this issue from ljharb/repo-report Aug 20, 2021
@Green-Ranger11
Copy link
Collaborator

@ljharb Anything else needed on this issue or can we close?

@ljharb ljharb closed this as completed Jan 27, 2022
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

5 participants