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.

Sign up for GitHub

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: Refine The Atlantis VCS Logging Configuration #4285

Merged
merged 10 commits into from
Feb 26, 2024
+1,497 −1,168
Merged

feat: Refine The Atlantis VCS Logging Configuration #4285

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3271690
Refine VCS Logging
X-Guardian Feb 24, 2024
e48b1df
Remove github/gitlab client logger
X-Guardian Feb 24, 2024
07dde5e
Remove fmtLogSrc from instrumented_client and format
X-Guardian Feb 24, 2024
d30e10c
Add staticcheck lint exception for NewGitHubClient
X-Guardian Feb 24, 2024
7dd7e00
Fix tests
X-Guardian Feb 24, 2024
f0f201b
Fix tests
X-Guardian Feb 24, 2024
854e5fa
Fix tests
X-Guardian Feb 24, 2024
6e51104
Fix tests
X-Guardian Feb 24, 2024
e262358
Fix tests
X-Guardian Feb 24, 2024
ca4db40
Merge branch 'main' into vcs-logging-refinement
X-Guardian Feb 24, 2024
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
2 changes: 1 addition & 1 deletion server/controllers/api_controller.go
View file
Open in desktop
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)
GenPage marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
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
Open in desktop
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
Loading