Skip to content

Commit

Permalink
feat: Improve GitHub and GitLab Debug Logging (#3876)
Browse files Browse the repository at this point in the history
* Improve GitHub and GitLab Debug Logging

* Fix formatting

* Fix g.client.Repositories.Get debug message

* Update gitlab_client_test with logger

---------

Co-authored-by: PePe Amengual <[email protected]>
  • Loading branch information
X-Guardian and jamengual authored Oct 20, 2023
1 parent 22ab01e commit edc9bd9
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 33 deletions.
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

0 comments on commit edc9bd9

Please sign in to comment.