Correctly fetch Github PR status

[benwaffle]

Fetch everything that makes a PR green, including the Status API and the Checks API.

Fixes #107

[rgarcia]

@benwaffle thanks for the contribution! Didn't realize that there was a distinction between commit statuses and checks. The graphql addition is neat but I am hesitant to add it since we don't use graphql at Clever and so something like the query struct with all of it's graphql annotations would present a challenge for the team to understand and maintain. If there's a way to do this with the non-graphql API that would be preferred. Thanks again for this PR.

[benwaffle]

@robherley there's a REST API for this too, right? but you said it was expensive to call?

@rgarcia Alternatively, we can still use the GraphQL API but avoid the Go annotations - just using JSON directly - see here

[robherley]

👋 @benwaffle @rgarcia Unfortunately there isn't an equivalent REST API for CombinedStatus (CheckRun states + Status states)

Alternatively if you wanted to use purely REST there's the List check suites for a Git reference API, which will get you all the check suites for a specific commit, and that covers checks (including actions). But, it's paginated and a bit slower than using the GraphQL query. You'd still need to call the Statuses API that you are doing today, in combination with the new Checks call.

My vote would be to just use the GraphQL call 👍

[robherley]

@benwaffle Is there a reason your query is going off of the Pull Request number?

The PR's latest head SHA is already available from here:

microplane/sync/syncGithub.go

Line 26 in 724c03c

pr, _, err := client.PullRequests.Get(ctx, r.Owner, r.Name, po.PullRequestNumber)

[benwaffle]

no particular reason, using the PR number worked

[robherley]

@rgarcia @benwaffle I may be misunderstanding, but is the only reason why all the statuses are fetched in this function is to find the circle ci context?

microplane/push/push.go

Lines 146 to 160 in 724c03c

	var circleCIBuildURL string
	for _, status := range cs.Statuses {
	if status.Context == "ci/circleci: build" && status.TargetURL != nil {
	circleCIBuildURL = *status.TargetURL
	// url has lots of ugly tracking query params, get rid of them
	if parsedURL, err := url.Parse(circleCIBuildURL); err == nil {
	query := parsedURL.Query()
	query.Del("utm_campaign")
	query.Del("utm_medium")
	query.Del("utm_source")
	parsedURL.RawQuery = query.Encode()
	circleCIBuildURL = parsedURL.String()
	}
	}
	}

If so, it may be better to just query for that context specifically using the GraphQL API: https://docs.github.com/en/graphql/reference/objects#status

query {
  repository(owner: "Clever", name: "microplane") {
    object(oid: "0e085b8bfa579162b9d3b8400cec7bfaaa42866a") {
      ... on Commit {
        status {
          context(name: "ci/circleci: build") {
            context
            state
            targetUrl
          }
        }
      }
    }
  }
}

I've seen repositories with hundreds of statuses before, there's no guarantee that the first 10 here will capture the CircleCI context for every commit.

[benwaffle]

Yes, just to get the circle ci build. Not sure where that's used, but I suppose there would be value in displaying other CI URLs (travis, jenkins, gh actions, etc...)

[Enrico2]

Is this PR still needed? Discovering this repo now and looking into using it internally, we don't fear GraphQL and absolutely do need to check for all statuses and checks :)
I'm likely needing to fork this repo anyway to support forking a repo where a push fails due to a lack of write permissions.

Thanks!

[benwaffle]

It's still relevant, but the maintainers would like it to be written using the REST API instead. Personally I've just been opening and merging all the PRs manually when green, and that's been good enough for me.