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

feat: Improve GitHub and GitLab Debug Logging #3876

Merged
merged 6 commits into from
Oct 20, 2023
Merged
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
49 changes: 30 additions & 19 deletions server/events/vcs/github_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,8 @@ listloop:
time.Sleep(attemptDelay)
attemptDelay = 2*attemptDelay + 1*time.Second

g.logger.Debug("[attempt %d] GET /repos/%v/%v/pulls/%d/files", i+1, repo.Owner, repo.Name, pull.Num)
pageFiles, resp, err := g.client.PullRequests.ListFiles(g.ctx, repo.Owner, repo.Name, pull.Num, &opts)
g.logger.Debug("[attempt %d] GET /repos/%v/%v/pulls/%d/files returned: %v", i+1, repo.Owner, repo.Name, pull.Num, resp.StatusCode)
if err != nil {
ghErr, ok := err.(*github.ErrorResponse)
if ok && ghErr.Response.StatusCode == 404 {
Expand Down Expand Up @@ -189,8 +189,8 @@ func (g *GithubClient) CreateComment(repo models.Repo, pullNum int, comment stri

comments := common.SplitComment(comment, maxCommentLength, sepEnd, sepStart)
for i := range comments {
g.logger.Debug("POST /repos/%v/%v/issues/%d/comments", repo.Owner, repo.Name, pullNum)
_, _, err := g.client.Issues.CreateComment(g.ctx, repo.Owner, repo.Name, pullNum, &github.IssueComment{Body: &comments[i]})
_, resp, err := g.client.Issues.CreateComment(g.ctx, repo.Owner, repo.Name, pullNum, &github.IssueComment{Body: &comments[i]})
g.logger.Debug("POST /repos/%v/%v/issues/%d/comments returned: %v", repo.Owner, repo.Name, pullNum, resp.StatusCode)
if err != nil {
return err
}
Expand All @@ -200,21 +200,21 @@ func (g *GithubClient) CreateComment(repo models.Repo, pullNum int, comment stri

// ReactToComment adds a reaction to a comment.
func (g *GithubClient) ReactToComment(repo models.Repo, pullNum int, commentID int64, reaction string) error {
g.logger.Debug("POST /repos/%v/%v/issues/comments/%d/reactions", repo.Owner, repo.Name, commentID)
_, _, err := g.client.Reactions.CreateIssueCommentReaction(g.ctx, repo.Owner, repo.Name, commentID, reaction)
_, resp, err := g.client.Reactions.CreateIssueCommentReaction(g.ctx, repo.Owner, repo.Name, commentID, reaction)
g.logger.Debug("POST /repos/%v/%v/issues/comments/%d/reactions returned: %v", repo.Owner, repo.Name, commentID, resp.StatusCode)
return err
}

func (g *GithubClient) HidePrevCommandComments(repo models.Repo, pullNum int, command string) error {
var allComments []*github.IssueComment
nextPage := 0
for {
g.logger.Debug("GET /repos/%v/%v/issues/%d/comments", repo.Owner, repo.Name, pullNum)
comments, resp, err := g.client.Issues.ListComments(g.ctx, repo.Owner, repo.Name, pullNum, &github.IssueListCommentsOptions{
Sort: github.String("created"),
Direction: github.String("asc"),
ListOptions: github.ListOptions{Page: nextPage},
})
g.logger.Debug("GET /repos/%v/%v/issues/%d/comments returned: %v", repo.Owner, repo.Name, pullNum, resp.StatusCode)
if err != nil {
return errors.Wrap(err, "listing comments")
}
Expand Down Expand Up @@ -327,8 +327,8 @@ func (g *GithubClient) PullIsApproved(repo models.Repo, pull models.PullRequest)
if nextPage != 0 {
opts.Page = nextPage
}
g.logger.Debug("GET /repos/%v/%v/pulls/%d/reviews", repo.Owner, repo.Name, pull.Num)
pageReviews, resp, err := g.client.PullRequests.ListReviews(g.ctx, repo.Owner, repo.Name, pull.Num, &opts)
g.logger.Debug("GET /repos/%v/%v/pulls/%d/reviews returned: %v", repo.Owner, repo.Name, pull.Num, resp.StatusCode)
if err != nil {
return approvalStatus, errors.Wrap(err, "getting reviews")
}
Expand Down Expand Up @@ -397,7 +397,8 @@ func isRequiredCheck(check string, required []string) bool {
// GetCombinedStatusMinusApply checks Statuses for PR, excluding atlantis apply. Returns true if all other statuses are not in failure.
func (g *GithubClient) GetCombinedStatusMinusApply(repo models.Repo, pull *github.PullRequest, vcstatusname string) (bool, error) {
//check combined status api
status, _, err := g.client.Repositories.GetCombinedStatus(g.ctx, *pull.Head.Repo.Owner.Login, repo.Name, *pull.Head.Ref, nil)
status, resp, err := g.client.Repositories.GetCombinedStatus(g.ctx, *pull.Head.Repo.Owner.Login, repo.Name, *pull.Head.Ref, nil)
g.logger.Debug("GET /repos/%v/%v/commits/%s/status returned: %v", *pull.Head.Repo.Owner.Login, repo.Name, *pull.Head.Ref, resp.StatusCode)
if err != nil {
return false, errors.Wrap(err, "getting combined status")
}
Expand All @@ -413,7 +414,8 @@ func (g *GithubClient) GetCombinedStatusMinusApply(repo models.Repo, pull *githu
}

//get required status checks
required, _, err := g.client.Repositories.GetBranchProtection(context.Background(), repo.Owner, repo.Name, *pull.Base.Ref)
required, resp, err := g.client.Repositories.GetBranchProtection(context.Background(), repo.Owner, repo.Name, *pull.Base.Ref)
g.logger.Debug("GET /repos/%v/%v/branches/%s/protection returned: %v", repo.Owner, repo.Name, *pull.Base.Ref, resp.StatusCode)
if err != nil {
return false, errors.Wrap(err, "getting required status checks")
}
Expand All @@ -423,7 +425,8 @@ func (g *GithubClient) GetCombinedStatusMinusApply(repo models.Repo, pull *githu
}

//check check suite/check run api
checksuites, _, err := g.client.Checks.ListCheckSuitesForRef(context.Background(), *pull.Head.Repo.Owner.Login, repo.Name, *pull.Head.Ref, nil)
checksuites, resp, err := g.client.Checks.ListCheckSuitesForRef(context.Background(), *pull.Head.Repo.Owner.Login, repo.Name, *pull.Head.Ref, nil)
g.logger.Debug("GET /repos/%v/%v/commits/%s/check-suites returned: %v", *pull.Head.Repo.Owner.Login, repo.Name, *pull.Head.Ref, resp.StatusCode)
if err != nil {
return false, errors.Wrap(err, "getting check suites for ref")
}
Expand All @@ -432,7 +435,8 @@ func (g *GithubClient) GetCombinedStatusMinusApply(repo models.Repo, pull *githu
for _, c := range checksuites.CheckSuites {
if *c.Status == "completed" {
//iterate over the runs inside the suite
suite, _, err := g.client.Checks.ListCheckRunsCheckSuite(context.Background(), *pull.Head.Repo.Owner.Login, repo.Name, *c.ID, nil)
suite, resp, err := g.client.Checks.ListCheckRunsCheckSuite(context.Background(), *pull.Head.Repo.Owner.Login, repo.Name, *c.ID, nil)
g.logger.Debug("GET /repos/%v/%v/check-suites/%d/check-runs returned: %v", *pull.Head.Repo.Owner.Login, repo.Name, *c.ID, resp.StatusCode)
if err != nil {
return false, errors.Wrap(err, "getting check runs for check suite")
}
Expand Down Expand Up @@ -546,7 +550,8 @@ func (g *GithubClient) GetPullRequest(repo models.Repo, num int) (*github.PullRe
time.Sleep(attemptDelay)
attemptDelay = 2*attemptDelay + 1*time.Second

pull, _, err = g.client.PullRequests.Get(g.ctx, repo.Owner, repo.Name, num)
pull, resp, err := g.client.PullRequests.Get(g.ctx, repo.Owner, repo.Name, num)
g.logger.Debug("GET /repos/%v/%v/pulls/%d returned: %v", repo.Owner, repo.Name, num, resp.StatusCode)
if err == nil {
return pull, nil
}
Expand Down Expand Up @@ -577,16 +582,17 @@ func (g *GithubClient) UpdateStatus(repo models.Repo, pull models.PullRequest, s
Context: github.String(src),
TargetURL: &url,
}
_, _, err := g.client.Repositories.CreateStatus(g.ctx, repo.Owner, repo.Name, pull.HeadCommit, status)
_, resp, err := g.client.Repositories.CreateStatus(g.ctx, repo.Owner, repo.Name, pull.HeadCommit, status)
g.logger.Debug("POST /repos/%v/%v/statuses/%s returned: %v", repo.Owner, repo.Name, pull.HeadCommit, resp.StatusCode)
return err
}

// MergePull merges the pull request.
func (g *GithubClient) MergePull(pull models.PullRequest, pullOptions models.PullRequestOptions) error {
// Users can set their repo to disallow certain types of merging.
// We detect which types aren't allowed and use the type that is.
g.logger.Debug("GET /repos/%v/%v", pull.BaseRepo.Owner, pull.BaseRepo.Name)
repo, _, err := g.client.Repositories.Get(g.ctx, pull.BaseRepo.Owner, pull.BaseRepo.Name)
repo, resp, err := g.client.Repositories.Get(g.ctx, pull.BaseRepo.Owner, pull.BaseRepo.Name)
g.logger.Debug("GET /repos/%v/%v returned: %v", pull.BaseRepo.Owner, pull.BaseRepo.Name, resp.StatusCode)
if err != nil {
return errors.Wrap(err, "fetching repo info")
}
Expand All @@ -609,7 +615,7 @@ func (g *GithubClient) MergePull(pull models.PullRequest, pullOptions models.Pul
MergeMethod: method,
}
g.logger.Debug("PUT /repos/%v/%v/pulls/%d/merge", repo.Owner, repo.Name, pull.Num)
mergeResult, _, err := g.client.PullRequests.Merge(
mergeResult, resp, err := g.client.PullRequests.Merge(
g.ctx,
pull.BaseRepo.Owner,
pull.BaseRepo.Name,
Expand All @@ -618,6 +624,7 @@ func (g *GithubClient) MergePull(pull models.PullRequest, pullOptions models.Pul
// the commit message as it normally would.
"",
options)
g.logger.Debug("POST /repos/%v/%v/pulls/%d/merge returned: %v", repo.Owner, repo.Name, pull.Num, resp.StatusCode)
if err != nil {
return errors.Wrap(err, "merging pull request")
}
Expand Down Expand Up @@ -678,7 +685,8 @@ func (g *GithubClient) GetTeamNamesForUser(repo models.Repo, user models.User) (
// ExchangeCode returns a newly created app's info
func (g *GithubClient) ExchangeCode(code string) (*GithubAppTemporarySecrets, error) {
ctx := context.Background()
cfg, _, err := g.client.Apps.CompleteAppManifest(ctx, code)
cfg, resp, err := g.client.Apps.CompleteAppManifest(ctx, code)
g.logger.Debug("POST /app-manifests/%s/conversions returned: %v", code, resp.StatusCode)
data := &GithubAppTemporarySecrets{
ID: cfg.GetID(),
Key: cfg.GetPEM(),
Expand All @@ -696,6 +704,7 @@ func (g *GithubClient) ExchangeCode(code string) (*GithubAppTemporarySecrets, er
func (g *GithubClient) GetFileContent(pull models.PullRequest, fileName string) (bool, []byte, error) {
opt := github.RepositoryContentGetOptions{Ref: pull.HeadBranch}
fileContent, _, resp, err := g.client.Repositories.GetContents(g.ctx, pull.BaseRepo.Owner, pull.BaseRepo.Name, fileName, &opt)
g.logger.Debug("GET /repos/%v/%v/contents/%s returned: %v", pull.BaseRepo.Owner, pull.BaseRepo.Name, fileName, resp.StatusCode)

if resp.StatusCode == http.StatusNotFound {
return false, []byte{}, nil
Expand All @@ -718,15 +727,17 @@ func (g *GithubClient) SupportsSingleFileDownload(repo models.Repo) bool {

func (g *GithubClient) GetCloneURL(VCSHostType models.VCSHostType, repo string) (string, error) {
parts := strings.Split(repo, "/")
repository, _, err := g.client.Repositories.Get(g.ctx, parts[0], parts[1])
repository, resp, err := g.client.Repositories.Get(g.ctx, parts[0], parts[1])
g.logger.Debug("GET /repos/%v/%v returned: %v", parts[0], parts[1], resp.StatusCode)
if err != nil {
return "", err
}
return repository.GetCloneURL(), nil
}

func (g *GithubClient) GetPullLabels(repo models.Repo, pull models.PullRequest) ([]string, error) {
pullDetails, _, err := g.client.PullRequests.Get(g.ctx, repo.Owner, repo.Name, pull.Num)
pullDetails, resp, err := g.client.PullRequests.Get(g.ctx, repo.Owner, repo.Name, pull.Num)
g.logger.Debug("GET /repos/%v/%v/pulls/%d returned: %v", repo.Owner, repo.Name, pull.Num, resp.StatusCode)
if err != nil {
return nil, err
}
Expand Down
46 changes: 32 additions & 14 deletions server/events/vcs/gitlab_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ func (g *GitlabClient) GetModifiedFiles(repo models.Repo, pull models.PullReques
pollingStart := time.Now()
for {
resp, err = g.Client.Do(req, mr)
g.logger.Debug("GET %s returned: %d", apiURL, resp.StatusCode)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -177,7 +178,9 @@ func (g *GitlabClient) CreateComment(repo models.Repo, pullNum int, comment stri
"```diff\n"
comments := common.SplitComment(comment, gitlabMaxCommentLength, sepEnd, sepStart)
for _, c := range comments {
if _, _, err := g.Client.Notes.CreateMergeRequestNote(repo.FullName, pullNum, &gitlab.CreateMergeRequestNoteOptions{Body: gitlab.String(c)}); err != nil {
_, resp, err := g.Client.Notes.CreateMergeRequestNote(repo.FullName, pullNum, &gitlab.CreateMergeRequestNoteOptions{Body: gitlab.String(c)})
g.logger.Debug("POST /projects/%s/merge_requests/%d/notes returned: %d", repo.FullName, pullNum, resp.StatusCode)
if err != nil {
return err
}
}
Expand All @@ -186,7 +189,8 @@ func (g *GitlabClient) CreateComment(repo models.Repo, pullNum int, comment stri

// ReactToComment adds a reaction to a comment.
func (g *GitlabClient) ReactToComment(repo models.Repo, pullNum int, commentID int64, reaction string) error {
_, _, err := g.Client.AwardEmoji.CreateMergeRequestAwardEmojiOnNote(repo.FullName, pullNum, int(commentID), &gitlab.CreateAwardEmojiOptions{Name: reaction})
_, resp, err := g.Client.AwardEmoji.CreateMergeRequestAwardEmojiOnNote(repo.FullName, pullNum, int(commentID), &gitlab.CreateAwardEmojiOptions{Name: reaction})
g.logger.Debug("POST /projects/%s/merge_requests/%d/notes/%d/award_emoji returned: %d", repo.FullName, pullNum, commentID, resp.StatusCode)
return err
}

Expand All @@ -202,6 +206,7 @@ func (g *GitlabClient) HidePrevCommandComments(repo models.Repo, pullNum int, co
OrderBy: gitlab.String("created_at"),
ListOptions: gitlab.ListOptions{Page: nextPage},
})
g.logger.Debug("GET /projects/%s/merge_requests/%d/notes returned: %d", repo.FullName, pullNum, resp.StatusCode)
if err != nil {
return errors.Wrap(err, "listing comments")
}
Expand Down Expand Up @@ -240,8 +245,9 @@ func (g *GitlabClient) HidePrevCommandComments(repo models.Repo, pullNum int, co
g.logger.Debug("Updating merge request note: Repo: '%s', MR: '%d', comment ID: '%d'", repo.FullName, pullNum, comment.ID)
supersededComment := summaryHeader + lineFeed + comment.Body + lineFeed + summaryFooter + lineFeed

if _, _, err := g.Client.Notes.UpdateMergeRequestNote(repo.FullName, pullNum, comment.ID,
&gitlab.UpdateMergeRequestNoteOptions{Body: &supersededComment}); err != nil {
_, resp, err := g.Client.Notes.UpdateMergeRequestNote(repo.FullName, pullNum, comment.ID, &gitlab.UpdateMergeRequestNoteOptions{Body: &supersededComment})
g.logger.Debug("PUT /projects/%s/merge_requests/%d/notes/%d returned: %d", repo.FullName, pullNum, comment.ID, resp.StatusCode)
if err != nil {
return errors.Wrapf(err, "updating comment %d", comment.ID)
}
}
Expand All @@ -251,7 +257,8 @@ func (g *GitlabClient) HidePrevCommandComments(repo models.Repo, pullNum int, co

// PullIsApproved returns true if the merge request was approved.
func (g *GitlabClient) PullIsApproved(repo models.Repo, pull models.PullRequest) (approvalStatus models.ApprovalStatus, err error) {
approvals, _, err := g.Client.MergeRequests.GetMergeRequestApprovals(repo.FullName, pull.Num)
approvals, resp, err := g.Client.MergeRequests.GetMergeRequestApprovals(repo.FullName, pull.Num)
g.logger.Debug("GET /projects/%s/merge_requests/%d/approvals returned: %d", repo.FullName, pull.Num, resp.StatusCode)
if err != nil {
return approvalStatus, err
}
Expand All @@ -275,7 +282,8 @@ func (g *GitlabClient) PullIsApproved(repo models.Repo, pull models.PullRequest)
// - https://gitlab.com/gitlab-org/gitlab-ee/issues/3169
// - https://gitlab.com/gitlab-org/gitlab-ce/issues/42344
func (g *GitlabClient) PullIsMergeable(repo models.Repo, pull models.PullRequest, vcsstatusname string) (bool, error) {
mr, _, err := g.Client.MergeRequests.GetMergeRequest(repo.FullName, pull.Num, nil)
mr, resp, err := g.Client.MergeRequests.GetMergeRequest(repo.FullName, pull.Num, nil)
g.logger.Debug("GET /projects/%s/merge_requests/%d returned: %d", repo.FullName, pull.Num, resp.StatusCode)
if err != nil {
return false, err
}
Expand All @@ -290,13 +298,15 @@ func (g *GitlabClient) PullIsMergeable(repo models.Repo, pull models.PullRequest
}

// Get project configuration
project, _, err := g.Client.Projects.GetProject(mr.ProjectID, nil)
project, resp, err := g.Client.Projects.GetProject(mr.ProjectID, nil)
g.logger.Debug("GET /projects/%d returned: %d", mr.ProjectID, resp.StatusCode)
if err != nil {
return false, err
}

// Get Commit Statuses
statuses, _, err := g.Client.Commits.GetCommitStatuses(mr.ProjectID, commit, nil)
g.logger.Debug("GET /projects/%d/commits/%s/statuses returned: %d", mr.ProjectID, commit, resp.StatusCode)
if err != nil {
return false, err
}
Expand Down Expand Up @@ -367,18 +377,20 @@ func (g *GitlabClient) UpdateStatus(repo models.Repo, pull models.PullRequest, s
refTarget = fmt.Sprintf("refs/merge-requests/%d/head", pull.Num)
}
}
_, _, err = g.Client.Commits.SetCommitStatus(repo.FullName, pull.HeadCommit, &gitlab.SetCommitStatusOptions{
_, resp, err := g.Client.Commits.SetCommitStatus(repo.FullName, pull.HeadCommit, &gitlab.SetCommitStatusOptions{
State: gitlabState,
Context: gitlab.String(src),
Description: gitlab.String(description),
TargetURL: &url,
Ref: gitlab.String(refTarget),
})
g.logger.Debug("POST /projects/%s/statuses/%s returned: %d", repo.FullName, pull.HeadCommit, resp.StatusCode)
return err
}

func (g *GitlabClient) GetMergeRequest(repoFullName string, pullNum int) (*gitlab.MergeRequest, error) {
mr, _, err := g.Client.MergeRequests.GetMergeRequest(repoFullName, pullNum, nil)
mr, resp, err := g.Client.MergeRequests.GetMergeRequest(repoFullName, pullNum, nil)
g.logger.Debug("GET /projects/%s/merge_requests/%d returned: %d", repoFullName, pullNum, resp.StatusCode)
return mr, err
}

Expand Down Expand Up @@ -413,7 +425,8 @@ func (g *GitlabClient) MergePull(pull models.PullRequest, pullOptions models.Pul
return errors.Wrap(
err, "unable to merge merge request, it was not possible to retrieve the merge request")
}
project, _, err := g.Client.Projects.GetProject(mr.ProjectID, nil)
project, resp, err := g.Client.Projects.GetProject(mr.ProjectID, nil)
g.logger.Debug("GET /projects/%d returned: %d", mr.ProjectID, resp.StatusCode)
if err != nil {
return errors.Wrap(
err, "unable to merge merge request, it was not possible to check the project requirements")
Expand All @@ -423,13 +436,14 @@ func (g *GitlabClient) MergePull(pull models.PullRequest, pullOptions models.Pul
g.WaitForSuccessPipeline(context.Background(), pull)
}

_, _, err = g.Client.MergeRequests.AcceptMergeRequest(
_, resp, err = g.Client.MergeRequests.AcceptMergeRequest(
pull.BaseRepo.FullName,
pull.Num,
&gitlab.AcceptMergeRequestOptions{
MergeCommitMessage: &commitMsg,
ShouldRemoveSourceBranch: &pullOptions.DeleteSourceBranchOnMerge,
})
g.logger.Debug("PUT /projects/%s/merge_requests/%d/merge returned: %d", pull.BaseRepo.FullName, pull.Num, resp.StatusCode)
return errors.Wrap(err, "unable to merge merge request, it may not be in a mergeable state")
}

Expand All @@ -445,7 +459,8 @@ func (g *GitlabClient) DiscardReviews(repo models.Repo, pull models.PullRequest)

// GetVersion returns the version of the Gitlab server this client is using.
func (g *GitlabClient) GetVersion() (*version.Version, error) {
versionResp, _, err := g.Client.Version.GetVersion()
versionResp, resp, err := g.Client.Version.GetVersion()
g.logger.Debug("GET /version returned: %d", resp.StatusCode)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -493,6 +508,7 @@ func (g *GitlabClient) GetFileContent(pull models.PullRequest, fileName string)
opt := gitlab.GetRawFileOptions{Ref: gitlab.String(pull.HeadBranch)}

bytes, resp, err := g.Client.RepositoryFiles.GetRawFile(pull.BaseRepo.FullName, fileName, &opt)
g.logger.Debug("GET /projects/%s/repository/files/%s/raw returned: %d", pull.BaseRepo.FullName, fileName, resp.StatusCode)
if resp.StatusCode == http.StatusNotFound {
return false, []byte{}, nil
}
Expand All @@ -509,15 +525,17 @@ func (g *GitlabClient) SupportsSingleFileDownload(repo models.Repo) bool {
}

func (g *GitlabClient) GetCloneURL(VCSHostType models.VCSHostType, repo string) (string, error) {
project, _, err := g.Client.Projects.GetProject(repo, nil)
project, resp, err := g.Client.Projects.GetProject(repo, nil)
g.logger.Debug("GET /projects/%s returned: %d", repo, resp.StatusCode)
if err != nil {
return "", err
}
return project.HTTPURLToRepo, nil
}

func (g *GitlabClient) GetPullLabels(repo models.Repo, pull models.PullRequest) ([]string, error) {
mr, _, err := g.Client.MergeRequests.GetMergeRequest(repo.FullName, pull.Num, nil)
mr, resp, err := g.Client.MergeRequests.GetMergeRequest(repo.FullName, pull.Num, nil)
g.logger.Debug("GET /projects/%s/merge_requests/%d returned: %d", repo.FullName, pull.Num, resp.StatusCode)

if err != nil {
return nil, err
Expand Down
Loading