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

Correctly fetch Github PR status #160

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,6 @@ release:
-o="$@/$(EXECUTABLE)-$(VERSION)-darwin-amd64"

install_deps:
go get github.com/GeertJohan/fgt@262f7b11eec07dc7b147c44641236f3212fee89d
go get golang.org/x/lint/golint@738671d3881b9731cc63024d5d88cf28db875626
go install github.com/GeertJohan/fgt@262f7b11eec07dc7b147c44641236f3212fee89d
go install golang.org/x/lint/golint@738671d3881b9731cc63024d5d88cf28db875626
go mod vendor
12 changes: 10 additions & 2 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ require (
github.com/facebookgo/errgroup v0.0.0-20160209021148-779c8d7ef069
github.com/fatih/color v1.13.0
github.com/google/go-github/v35 v35.3.0
github.com/hasura/go-graphql-client v0.7.0
github.com/spf13/cobra v1.4.0
github.com/stretchr/testify v1.7.2
github.com/waigani/diffparser v0.0.0-20190828052634-7391f219313d
Expand All @@ -15,29 +16,36 @@ require (
)

require (
github.com/GeertJohan/fgt v0.0.0-20160120143236-262f7b11eec0 // indirect
github.com/cpuguy83/go-md2man/v2 v2.0.1 // indirect
github.com/davecgh/go-spew v1.1.1 // indirect
github.com/facebookgo/ensure v0.0.0-20200202191622-63f1cf65ac4c // indirect
github.com/facebookgo/stack v0.0.0-20160209184415-751773369052 // indirect
github.com/facebookgo/subset v0.0.0-20200203212716-c811ad88dec4 // indirect
github.com/golang/protobuf v1.5.2 // indirect
github.com/google/go-cmp v0.5.7 // indirect
github.com/google/go-querystring v1.1.0 // indirect
github.com/google/uuid v1.3.0 // indirect
github.com/hashicorp/go-cleanhttp v0.5.2 // indirect
github.com/hashicorp/go-retryablehttp v0.7.1 // indirect
github.com/inconshreveable/mousetrap v1.0.0 // indirect
github.com/klauspost/compress v1.15.6 // indirect
github.com/kr/text v0.2.0 // indirect
github.com/mattn/go-colorable v0.1.12 // indirect
github.com/mattn/go-isatty v0.0.14 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/russross/blackfriday/v2 v2.1.0 // indirect
github.com/spf13/pflag v1.0.5 // indirect
golang.org/x/crypto v0.0.0-20210817164053-32db794688a5 // indirect
golang.org/x/crypto v0.0.0-20220408190544-5352b0902921 // indirect
golang.org/x/lint v0.0.0-20200302205851-738671d3881b // indirect
golang.org/x/net v0.0.0-20220520000938-2e3eb7b945c2 // indirect
golang.org/x/sys v0.0.0-20211216021012-1d35b9e2eb4e // indirect
golang.org/x/sys v0.0.0-20220406163625-3f8b81556e12 // indirect
golang.org/x/time v0.0.0-20220411224347-583f2d630306 // indirect
golang.org/x/tools v0.0.0-20200825202427-b303f430e36d // indirect
google.golang.org/appengine v1.6.7 // indirect
google.golang.org/protobuf v1.28.0 // indirect
gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c // indirect
gopkg.in/yaml.v2 v2.4.0 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
nhooyr.io/websocket v1.8.7 // indirect
)
63 changes: 58 additions & 5 deletions go.sum

Large diffs are not rendered by default.

6 changes: 3 additions & 3 deletions golang.mk
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export TZ=UTC
GO_BUILD_FLAGS := "-mod=vendor"

# if the gopath includes several directories, use only the first
GOPATH=$(shell echo $$GOPATH | cut -d: -f1)
GOPATH=$(shell go env GOPATH | cut -d: -f1)

# This block checks and confirms that the proper Go toolchain version is installed.
# It uses ^ matching in the semver sense -- you can be ahead by a minor
Expand All @@ -39,7 +39,7 @@ endef
# so we're defended against it breaking or changing in the future.
FGT := $(GOPATH)/bin/fgt
$(FGT):
go get github.com/GeertJohan/fgt@262f7b11eec07dc7b147c44641236f3212fee89d
go install github.com/GeertJohan/fgt@262f7b11eec07dc7b147c44641236f3212fee89d

golang-ensure-curl-installed:
@command -v curl >/dev/null 2>&1 || { echo >&2 "curl not installed. Please install curl."; exit 1; }
Expand All @@ -49,7 +49,7 @@ golang-ensure-curl-installed:
# previously passing tests start failing without changing our code.
GOLINT := $(GOPATH)/bin/golint
$(GOLINT):
go get golang.org/x/lint/golint@738671d3881b9731cc63024d5d88cf28db875626
go install golang.org/x/lint/golint@738671d3881b9731cc63024d5d88cf28db875626

# golang-fmt-deps requires the FGT tool for checking output
golang-fmt-deps: $(FGT)
Expand Down
91 changes: 91 additions & 0 deletions lib/githubPRStatus.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
package lib

import (
"context"
"fmt"
"strings"
"time"

"github.com/google/go-github/v35/github"
"github.com/hasura/go-graphql-client"
)

type GithubStatusContext struct {
Context string
TargetURL *string
}

type GithubPRStatus struct {
// "error", "expected", "failure", "pending", or "success"
State string

// Results from the Status Checks. Does not include results from the Checks API (incl. Github Actions)
Statuses []GithubStatusContext
}

func GetGithubPRStatus(ctx context.Context, client *github.Client, repoLimiter *time.Ticker, repo Repo, prNumber int) (GithubPRStatus, error) {
p := NewProviderFromConfig(repo.ProviderConfig)
graphqlClient, err := p.GithubGraphqlClient(ctx, client)
if err != nil {
return GithubPRStatus{}, err
}

// Fetch the rolled up status checks from github
// This includes both the Statuses API and Checks API
var query struct {
Repository struct {
PullRequest struct {
Commits struct {
Nodes []struct {
Commit struct {
StatusCheckRollup struct {
State string
Contexts struct {
Nodes []struct {
Typename string `graphql:"__typename"`
StatusContext struct {
Context string
TargetURL string
} `graphql:"... on StatusContext"`
}
} `graphql:"contexts(first: 10)"`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...)

}
}
}
} `graphql:"commits(last: 1)"`
} `graphql:"pullRequest(number: $number)"`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no particular reason, using the PR number worked

} `graphql:"repository(owner: $owner, name: $name)"`
}

<-repoLimiter.C
err = graphqlClient.Query(ctx, &query, map[string]interface{}{
"owner": graphql.String(repo.Owner),
"name": graphql.String(repo.Name),
"number": graphql.Int(prNumber),
})
if err != nil {
return GithubPRStatus{}, err
}

commits := query.Repository.PullRequest.Commits.Nodes
if len(commits) != 1 {
return GithubPRStatus{}, fmt.Errorf("unexpected number of commits in PR")
}

lastCommit := commits[0].Commit.StatusCheckRollup
var statuses []GithubStatusContext

for _, status := range lastCommit.Contexts.Nodes {
if status.Typename == "StatusContext" {
statuses = append(statuses, GithubStatusContext{
Context: status.StatusContext.Context,
TargetURL: &status.StatusContext.TargetURL,
})
}
}

return GithubPRStatus{
State: strings.ToLower(lastCommit.State),
Statuses: statuses,
}, nil
}
15 changes: 15 additions & 0 deletions lib/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@ package lib
import (
"context"
"fmt"
"net/http"
"os"

"github.com/google/go-github/v35/github"
"github.com/hasura/go-graphql-client"
"github.com/xanzy/go-gitlab"
"golang.org/x/oauth2"
)
Expand Down Expand Up @@ -51,6 +53,19 @@ func (p *Provider) GithubClient(ctx context.Context) (*github.Client, error) {
return client, nil
}

func (p *Provider) GithubGraphqlClient(ctx context.Context, rest *github.Client) (*graphql.Client, error) {
token := os.Getenv("GITHUB_API_TOKEN")
if token == "" {
return nil, fmt.Errorf("cannot initialize GithubGraphqlClient: GITHUB_API_TOKEN is not set")
}

client := graphql.NewClient(rest.BaseURL.String()+"graphql", nil).
WithRequestModifier(func(req *http.Request) {
req.Header.Add("Authorization", "Bearer "+token)
})
return client, nil
}

func (p *Provider) GitlabClient() (*gitlab.Client, error) {
// validation
if p.Backend != "gitlab" {
Expand Down
8 changes: 3 additions & 5 deletions merge/merge.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,16 +69,14 @@ func GitHubMerge(ctx context.Context, input Input, repoLimiter *time.Ticker, mer
}

// (2) Check commit status
<-repoLimiter.C
status, _, err := client.Repositories.GetCombinedStatus(ctx, input.Repo.Owner, input.Repo.Name, input.CommitSHA, &github.ListOptions{})
status, err := lib.GetGithubPRStatus(ctx, client, repoLimiter, input.Repo, input.PRNumber)
if err != nil {
return Output{Success: false}, err
}

if input.RequireBuildSuccess {
state := status.GetState()
if state != "success" {
return Output{Success: false}, fmt.Errorf("Build status was not 'success', instead was '%s'. Use --ignore-build-status to override this check.", state)
if status.State != "success" {
return Output{Success: false}, fmt.Errorf("Build status was not 'success', instead was '%s'. Use --ignore-build-status to override this check.", status.State)
}
}

Expand Down
7 changes: 3 additions & 4 deletions push/push.go
Original file line number Diff line number Diff line change
Expand Up @@ -138,15 +138,14 @@ func GithubPush(ctx context.Context, input Input, repoLimiter *time.Ticker, push
}
}

<-repoLimiter.C
cs, _, err := client.Repositories.GetCombinedStatus(ctx, input.Repo.Owner, input.Repo.Name, *pr.Head.SHA, nil)
cs, err := lib.GetGithubPRStatus(ctx, client, repoLimiter, input.Repo, *pr.Number)
if err != nil {
return Output{Success: false}, err
}

var circleCIBuildURL string
for _, status := range cs.Statuses {
if status.Context != nil && *status.Context == "ci/circleci" && status.TargetURL != nil {
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 {
Expand All @@ -165,7 +164,7 @@ func GithubPush(ctx context.Context, input Input, repoLimiter *time.Ticker, push
CommitSHA: *pr.Head.SHA,
PullRequestNumber: *pr.Number,
PullRequestURL: *pr.HTMLURL,
PullRequestCombinedStatus: *cs.State,
PullRequestCombinedStatus: cs.State,
PullRequestAssignee: input.PRAssignee,
CircleCIBuildURL: circleCIBuildURL,
}, nil
Expand Down
5 changes: 2 additions & 3 deletions sync/syncGithub.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,14 @@ func GithubSyncPush(ctx context.Context, r lib.Repo, po push.Output, repoLimiter
return Output{}, err
}

<-repoLimiter.C
cs, _, err := client.Repositories.GetCombinedStatus(ctx, r.Owner, r.Name, *pr.Head.SHA, nil)
cs, err := lib.GetGithubPRStatus(ctx, client, repoLimiter, r, po.PullRequestNumber)
if err != nil {
return Output{}, err
}

return Output{
CommitSHA: *pr.Head.SHA,
PullRequestCombinedStatus: *cs.State,
PullRequestCombinedStatus: cs.State,
MergeCommitSHA: *pr.MergeCommitSHA,
Merged: *pr.Merged,
}, nil
Expand Down