Skip to content

Commit

Permalink
feat: Refine The Atlantis VCS Logging Configuration (#4285)
Browse files Browse the repository at this point in the history 
* Refine VCS Logging

* Remove github/gitlab client logger

* Remove fmtLogSrc from instrumented_client and format

* Add staticcheck lint exception for NewGitHubClient

* Fix tests
  • Loading branch information
@X-Guardian
X-Guardian authored Feb 26, 2024
1 parent 0c99a3d commit 67b5740
Show file tree
Hide file tree
Showing 60 changed files with 1,497 additions and 1,168 deletions.
2 changes: 1 addition & 1 deletion server/controllers/api_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ func (a *APIController) apiParseAndValidate(r *http.Request) (*APIRequest, *comm
if err != nil {
return nil, nil, http.StatusBadRequest, err
}
cloneURL, err := a.VCSClient.GetCloneURL(VCSHostType, request.Repository)
cloneURL, err := a.VCSClient.GetCloneURL(a.Logger, VCSHostType, request.Repository)
if err != nil {
return nil, nil, http.StatusInternalServerError, err
}
Expand Down
17 changes: 11 additions & 6 deletions server/controllers/events/events_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ func (e *VCSEventsController) HandleGithubCommentEvent(event *github.IssueCommen
}
}

baseRepo, user, pullNum, err := e.Parser.ParseGithubIssueCommentEvent(event)
baseRepo, user, pullNum, err := e.Parser.ParseGithubIssueCommentEvent(logger, event)

wrapped := errors.Wrapf(err, "Failed parsing event: %s", githubReqID)
if err != nil {
Expand Down Expand Up @@ -409,7 +409,7 @@ func (e *VCSEventsController) handleBitbucketServerPullRequestEvent(w http.Respo
// request if the event is a pull request closed event. It's exported to make
// testing easier.
func (e *VCSEventsController) HandleGithubPullRequestEvent(logger logging.SimpleLogging, pullEvent *github.PullRequestEvent, githubReqID string) HTTPResponse {
pull, pullEventType, baseRepo, headRepo, user, err := e.Parser.ParseGithubPullEvent(pullEvent)
pull, pullEventType, baseRepo, headRepo, user, err := e.Parser.ParseGithubPullEvent(logger, pullEvent)
if err != nil {
wrapped := errors.Wrapf(err, "Error parsing pull data: %s %s", err, githubReqID)
return HTTPResponse{
Expand Down Expand Up @@ -465,7 +465,7 @@ func (e *VCSEventsController) handlePullRequestEvent(logger logging.SimpleLoggin
}
case models.ClosedPullEvent:
// If the pull request was closed, we delete locks.
if err := e.PullCleaner.CleanUpPull(baseRepo, pull); err != nil {
if err := e.PullCleaner.CleanUpPull(logger, baseRepo, pull); err != nil {
return HTTPResponse{
body: err.Error(),
err: HTTPError{
Expand Down Expand Up @@ -536,6 +536,11 @@ func (e *VCSEventsController) HandleGitlabCommentEvent(w http.ResponseWriter, ev
}

func (e *VCSEventsController) handleCommentEvent(logger logging.SimpleLogging, baseRepo models.Repo, maybeHeadRepo *models.Repo, maybePull *models.PullRequest, user models.User, pullNum int, comment string, commentID int64, vcsHost models.VCSHostType) HTTPResponse {
logger = logger.WithHistory(
"repo", baseRepo.FullName,
"pull", pullNum,
)

parseResult := e.CommentParser.Parse(comment, vcsHost)
if parseResult.Ignore {
truncated := comment
Expand Down Expand Up @@ -568,7 +573,7 @@ func (e *VCSEventsController) handleCommentEvent(logger logging.SimpleLogging, b

// It's a comment we're gonna react to, so add a reaction.
if e.EmojiReaction != "" {
err := e.VCSClient.ReactToComment(baseRepo, pullNum, commentID, e.EmojiReaction)
err := e.VCSClient.ReactToComment(logger, baseRepo, pullNum, commentID, e.EmojiReaction)
if err != nil {
logger.Warn("Failed to react to comment: %s", err)
}
Expand All @@ -579,7 +584,7 @@ func (e *VCSEventsController) handleCommentEvent(logger logging.SimpleLogging, b
// We do this here rather than earlier because we need access to the pull
// variable to comment back on the pull request.
if parseResult.CommentResponse != "" {
if err := e.VCSClient.CreateComment(baseRepo, pullNum, parseResult.CommentResponse, ""); err != nil {
if err := e.VCSClient.CreateComment(logger, baseRepo, pullNum, parseResult.CommentResponse, ""); err != nil {
logger.Err("unable to comment on pull request: %s", err)
}
return HTTPResponse{
Expand Down Expand Up @@ -762,7 +767,7 @@ func (e *VCSEventsController) commentNotAllowlisted(baseRepo models.Repo, pullNu
}

errMsg := "```\nError: This repo is not allowlisted for Atlantis.\n```"
if err := e.VCSClient.CreateComment(baseRepo, pullNum, errMsg, ""); err != nil {
if err := e.VCSClient.CreateComment(e.Logger, baseRepo, pullNum, errMsg, ""); err != nil {
e.Logger.Err("unable to comment on pull request: %s", err)
}
}
Expand Down
39 changes: 24 additions & 15 deletions server/controllers/events/events_controller_e2e_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -643,8 +643,10 @@ func TestGitHubWorkflow(t *testing.T) {

// Setup test dependencies.
w := httptest.NewRecorder()
When(githubGetter.GetPullRequest(Any[models.Repo](), Any[int]())).ThenReturn(GitHubPullRequestParsed(headSHA), nil)
When(vcsClient.GetModifiedFiles(Any[models.Repo](), Any[models.PullRequest]())).ThenReturn(c.ModifiedFiles, nil)
When(githubGetter.GetPullRequest(
Any[logging.SimpleLogging](), Any[models.Repo](), Any[int]())).ThenReturn(GitHubPullRequestParsed(headSHA), nil)
When(vcsClient.GetModifiedFiles(
Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest]())).ThenReturn(c.ModifiedFiles, nil)

// First, send the open pull request event which triggers autoplan.
pullOpenedReq := GitHubPullRequestOpenedEvent(t, headSHA)
Expand Down Expand Up @@ -707,17 +709,18 @@ func TestGitHubWorkflow(t *testing.T) {
expNumReplies++
}

_, _, actReplies, _ := vcsClient.VerifyWasCalled(Times(expNumReplies)).CreateComment(Any[models.Repo](), Any[int](), Any[string](), Any[string]()).GetAllCapturedArguments()
_, _, _, actReplies, _ := vcsClient.VerifyWasCalled(Times(expNumReplies)).CreateComment(
Any[logging.SimpleLogging](), Any[models.Repo](), Any[int](), Any[string](), Any[string]()).GetAllCapturedArguments()
Assert(t, len(c.ExpReplies) == len(actReplies), "missing expected replies, got %d but expected %d", len(actReplies), len(c.ExpReplies))
for i, expReply := range c.ExpReplies {
assertCommentEquals(t, expReply, actReplies[i], c.RepoDir, c.ExpParallel)
}

if c.ExpAutomerge {
// Verify that the merge API call was made.
vcsClient.VerifyWasCalledOnce().MergePull(Any[models.PullRequest](), Any[models.PullRequestOptions]())
vcsClient.VerifyWasCalledOnce().MergePull(Any[logging.SimpleLogging](), Any[models.PullRequest](), Any[models.PullRequestOptions]())
} else {
vcsClient.VerifyWasCalled(Never()).MergePull(Any[models.PullRequest](), Any[models.PullRequestOptions]())
vcsClient.VerifyWasCalled(Never()).MergePull(Any[logging.SimpleLogging](), Any[models.PullRequest](), Any[models.PullRequestOptions]())
}
})
}
Expand Down Expand Up @@ -819,8 +822,8 @@ func TestSimpleWorkflow_terraformLockFile(t *testing.T) {

// Setup test dependencies.
w := httptest.NewRecorder()
When(githubGetter.GetPullRequest(Any[models.Repo](), Any[int]())).ThenReturn(GitHubPullRequestParsed(headSHA), nil)
When(vcsClient.GetModifiedFiles(Any[models.Repo](), Any[models.PullRequest]())).ThenReturn(c.ModifiedFiles, nil)
When(githubGetter.GetPullRequest(Any[logging.SimpleLogging](), Any[models.Repo](), Any[int]())).ThenReturn(GitHubPullRequestParsed(headSHA), nil)
When(vcsClient.GetModifiedFiles(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest]())).ThenReturn(c.ModifiedFiles, nil)

// First, send the open pull request event which triggers autoplan.
pullOpenedReq := GitHubPullRequestOpenedEvent(t, headSHA)
Expand Down Expand Up @@ -880,7 +883,8 @@ func TestSimpleWorkflow_terraformLockFile(t *testing.T) {
// and apply have 1 for each comment plus one for the locks deleted at the
// end.

_, _, actReplies, _ := vcsClient.VerifyWasCalled(Times(2)).CreateComment(Any[models.Repo](), Any[int](), Any[string](), Any[string]()).GetAllCapturedArguments()
_, _, _, actReplies, _ := vcsClient.VerifyWasCalled(Times(2)).CreateComment(
Any[logging.SimpleLogging](), Any[models.Repo](), Any[int](), Any[string](), Any[string]()).GetAllCapturedArguments()
Assert(t, len(c.ExpReplies) == len(actReplies), "missing expected replies, got %d but expected %d", len(actReplies), len(c.ExpReplies))
for i, expReply := range c.ExpReplies {
assertCommentEquals(t, expReply, actReplies[i], c.RepoDir, false)
Expand Down Expand Up @@ -1188,12 +1192,16 @@ func TestGitHubWorkflowWithPolicyCheck(t *testing.T) {

// Setup test dependencies.
w := httptest.NewRecorder()
When(vcsClient.PullIsMergeable(Any[models.Repo](), Any[models.PullRequest](), Eq("atlantis-test"))).ThenReturn(true, nil)
When(vcsClient.PullIsApproved(Any[models.Repo](), Any[models.PullRequest]())).ThenReturn(models.ApprovalStatus{
When(vcsClient.PullIsMergeable(
Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), Eq("atlantis-test"))).ThenReturn(true, nil)
When(vcsClient.PullIsApproved(
Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest]())).ThenReturn(models.ApprovalStatus{
IsApproved: true,
}, nil)
When(githubGetter.GetPullRequest(Any[models.Repo](), Any[int]())).ThenReturn(GitHubPullRequestParsed(headSHA), nil)
When(vcsClient.GetModifiedFiles(Any[models.Repo](), Any[models.PullRequest]())).ThenReturn(c.ModifiedFiles, nil)
When(githubGetter.GetPullRequest(
Any[logging.SimpleLogging](), Any[models.Repo](), Any[int]())).ThenReturn(GitHubPullRequestParsed(headSHA), nil)
When(vcsClient.GetModifiedFiles(
Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest]())).ThenReturn(c.ModifiedFiles, nil)

// First, send the open pull request event which triggers autoplan.
pullOpenedReq := GitHubPullRequestOpenedEvent(t, headSHA)
Expand Down Expand Up @@ -1244,7 +1252,8 @@ func TestGitHubWorkflowWithPolicyCheck(t *testing.T) {
if !c.ExpPolicyChecks {
expNumReplies--
}
_, _, actReplies, _ := vcsClient.VerifyWasCalled(Times(expNumReplies)).CreateComment(Any[models.Repo](), Any[int](), Any[string](), Any[string]()).GetAllCapturedArguments()
_, _, _, actReplies, _ := vcsClient.VerifyWasCalled(Times(expNumReplies)).CreateComment(
Any[logging.SimpleLogging](), Any[models.Repo](), Any[int](), Any[string](), Any[string]()).GetAllCapturedArguments()

Assert(t, len(c.ExpReplies) == len(actReplies), "missing expected replies, got %d but expected %d", len(actReplies), len(c.ExpReplies))
for i, expReply := range c.ExpReplies {
Expand All @@ -1253,9 +1262,9 @@ func TestGitHubWorkflowWithPolicyCheck(t *testing.T) {

if c.ExpAutomerge {
// Verify that the merge API call was made.
vcsClient.VerifyWasCalledOnce().MergePull(Any[models.PullRequest](), Any[models.PullRequestOptions]())
vcsClient.VerifyWasCalledOnce().MergePull(Any[logging.SimpleLogging](), Any[models.PullRequest](), Any[models.PullRequestOptions]())
} else {
vcsClient.VerifyWasCalled(Never()).MergePull(Any[models.PullRequest](), Any[models.PullRequestOptions]())
vcsClient.VerifyWasCalled(Never()).MergePull(Any[logging.SimpleLogging](), Any[models.PullRequest](), Any[models.PullRequestOptions]())
}
})
}
Expand Down
Loading

0 comments on commit 67b5740

Please sign in to comment.