Skip to content

Commit

Permalink
Refine the logging in the working dir package
Browse files Browse the repository at this point in the history
  • Loading branch information
X-Guardian committed Feb 28, 2024
1 parent a3855e4 commit 7a9c7a9
Show file tree
Hide file tree
Showing 28 changed files with 668 additions and 617 deletions.
2 changes: 0 additions & 2 deletions server/controllers/events/events_controller_e2e_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1333,7 +1333,6 @@ func setupE2E(t *testing.T, repoDir string, opt setupOption) (events_controllers
workingDir := &events.FileWorkspace{
DataDir: dataDir,
TestingOverrideHeadCloneURL: "override-me",
Logger: logger,
}
var preWorkflowHooks []*valid.WorkflowHook
if !opt.disablePreWorkflowHooks {
Expand Down Expand Up @@ -1425,7 +1424,6 @@ func setupE2E(t *testing.T, repoDir string, opt setupOption) (events_controllers
false,
"auto",
statsScope,
logger,
terraformClient,
)

Expand Down
12 changes: 6 additions & 6 deletions server/controllers/locks_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ func (l *LocksController) GetLock(w http.ResponseWriter, r *http.Request) {
return
}
if lock == nil {
l.respond(w, logging.Info, http.StatusNotFound, "No lock found at id %q", idUnencoded)
l.respond(w, logging.Info, http.StatusNotFound, "No lock found at id '%s'", idUnencoded)
return
}

Expand Down Expand Up @@ -107,18 +107,18 @@ func (l *LocksController) DeleteLock(w http.ResponseWriter, r *http.Request) {

idUnencoded, err := url.PathUnescape(id)
if err != nil {
l.respond(w, logging.Warn, http.StatusBadRequest, "Invalid lock id %q. Failed with error: %s", id, err)
l.respond(w, logging.Warn, http.StatusBadRequest, "Invalid lock id '%s'. Failed with error: '%s'", id, err)
return
}

lock, err := l.DeleteLockCommand.DeleteLock(idUnencoded)
lock, err := l.DeleteLockCommand.DeleteLock(l.Logger, idUnencoded)
if err != nil {
l.respond(w, logging.Error, http.StatusInternalServerError, "deleting lock failed with: %s", err)
l.respond(w, logging.Error, http.StatusInternalServerError, "deleting lock failed with: '%s'", err)
return
}

if lock == nil {
l.respond(w, logging.Info, http.StatusNotFound, "No lock found at id %q", idUnencoded)
l.respond(w, logging.Info, http.StatusNotFound, "No lock found at id '%s'", idUnencoded)
return
}

Expand All @@ -139,7 +139,7 @@ func (l *LocksController) DeleteLock(w http.ResponseWriter, r *http.Request) {
} else {
l.Logger.Debug("skipping commenting on pull request and deleting workspace because BaseRepo field is empty")
}
l.respond(w, logging.Info, http.StatusOK, "Deleted lock id %q", id)
l.respond(w, logging.Info, http.StatusOK, "Deleted lock id '%s'", id)
}

// respond is a helper function to respond and log the response. lvl is the log
Expand Down
12 changes: 6 additions & 6 deletions server/controllers/locks_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ func TestDeleteLock_LockerErr(t *testing.T) {
t.Log("If there is an error retrieving the lock, a 500 is returned")
RegisterMockTestingT(t)
dlc := mocks2.NewMockDeleteLockCommand()
When(dlc.DeleteLock("id")).ThenReturn(nil, errors.New("err"))
When(dlc.DeleteLock(Any[logging.SimpleLogging](), Eq("id"))).ThenReturn(nil, errors.New("err"))
lc := controllers.LocksController{
DeleteLockCommand: dlc,
Logger: logging.NewNoopLogger(t),
Expand All @@ -238,7 +238,7 @@ func TestDeleteLock_None(t *testing.T) {
t.Log("If there is no lock at that ID we get a 404")
RegisterMockTestingT(t)
dlc := mocks2.NewMockDeleteLockCommand()
When(dlc.DeleteLock("id")).ThenReturn(nil, nil)
When(dlc.DeleteLock(Any[logging.SimpleLogging](), Eq("id"))).ThenReturn(nil, nil)
lc := controllers.LocksController{
DeleteLockCommand: dlc,
Logger: logging.NewNoopLogger(t),
Expand All @@ -255,7 +255,7 @@ func TestDeleteLock_OldFormat(t *testing.T) {
RegisterMockTestingT(t)
cp := vcsmocks.NewMockClient()
dlc := mocks2.NewMockDeleteLockCommand()
When(dlc.DeleteLock("id")).ThenReturn(&models.ProjectLock{}, nil)
When(dlc.DeleteLock(Any[logging.SimpleLogging](), Eq("id"))).ThenReturn(&models.ProjectLock{}, nil)
lc := controllers.LocksController{
DeleteLockCommand: dlc,
Logger: logging.NewNoopLogger(t),
Expand Down Expand Up @@ -284,7 +284,7 @@ func TestDeleteLock_UpdateProjectStatus(t *testing.T) {
pull := models.PullRequest{
BaseRepo: models.Repo{FullName: repoName},
}
When(l.DeleteLock("id")).ThenReturn(&models.ProjectLock{
When(l.DeleteLock(Any[logging.SimpleLogging](), Eq("id"))).ThenReturn(&models.ProjectLock{
Pull: pull,
Workspace: workspaceName,
Project: models.Project{
Expand Down Expand Up @@ -338,7 +338,7 @@ func TestDeleteLock_CommentFailed(t *testing.T) {
t.Log("If the commenting fails we still return success")
RegisterMockTestingT(t)
dlc := mocks2.NewMockDeleteLockCommand()
When(dlc.DeleteLock("id")).ThenReturn(&models.ProjectLock{
When(dlc.DeleteLock(Any[logging.SimpleLogging](), Eq("id"))).ThenReturn(&models.ProjectLock{
Pull: models.PullRequest{
BaseRepo: models.Repo{FullName: "owner/repo"},
},
Expand Down Expand Up @@ -380,7 +380,7 @@ func TestDeleteLock_CommentSuccess(t *testing.T) {
pull := models.PullRequest{
BaseRepo: models.Repo{FullName: "owner/repo"},
}
When(dlc.DeleteLock("id")).ThenReturn(&models.ProjectLock{
When(dlc.DeleteLock(Any[logging.SimpleLogging](), Eq("id"))).ThenReturn(&models.ProjectLock{
Pull: pull,
Workspace: "workspace",
Project: models.Project{
Expand Down
6 changes: 3 additions & 3 deletions server/events/command_requirement_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ func (a *DefaultCommandRequirementHandler) ValidatePlanProject(repoDir string, c
return "Pull request must be mergeable before running plan.", nil
}
case raw.UnDivergedRequirement:
if a.WorkingDir.HasDiverged(repoDir) {
if a.WorkingDir.HasDiverged(ctx.Log, repoDir) {
return "Default branch must be rebased onto pull request before running plan.", nil
}
}
Expand All @@ -60,7 +60,7 @@ func (a *DefaultCommandRequirementHandler) ValidateApplyProject(repoDir string,
return "Pull request must be mergeable before running apply.", nil
}
case raw.UnDivergedRequirement:
if a.WorkingDir.HasDiverged(repoDir) {
if a.WorkingDir.HasDiverged(ctx.Log, repoDir) {
return "Default branch must be rebased onto pull request before running apply.", nil
}
}
Expand Down Expand Up @@ -95,7 +95,7 @@ func (a *DefaultCommandRequirementHandler) ValidateImportProject(repoDir string,
return "Pull request must be mergeable before running import.", nil
}
case raw.UnDivergedRequirement:
if a.WorkingDir.HasDiverged(repoDir) {
if a.WorkingDir.HasDiverged(ctx.Log, repoDir) {
return "Default branch must be rebased onto pull request before running import.", nil
}
}
Expand Down
13 changes: 7 additions & 6 deletions server/events/command_requirement_handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/runatlantis/atlantis/server/core/config/valid"
"github.com/runatlantis/atlantis/server/events"
"github.com/runatlantis/atlantis/server/events/models"
"github.com/runatlantis/atlantis/server/logging"

"github.com/runatlantis/atlantis/server/events/command"
"github.com/runatlantis/atlantis/server/events/mocks"
Expand Down Expand Up @@ -46,7 +47,7 @@ func TestAggregateApplyRequirements_ValidatePlanProject(t *testing.T) {
ProjectPlanStatus: models.PassedPolicyCheckStatus,
},
setup: func(workingDir *mocks.MockWorkingDir) {
When(workingDir.HasDiverged(Any[string]())).ThenReturn(false)
When(workingDir.HasDiverged(Any[logging.SimpleLogging](), Any[string]())).ThenReturn(false)
},
wantErr: assert.NoError,
},
Expand Down Expand Up @@ -76,7 +77,7 @@ func TestAggregateApplyRequirements_ValidatePlanProject(t *testing.T) {
PlanRequirements: []string{raw.UnDivergedRequirement},
},
setup: func(workingDir *mocks.MockWorkingDir) {
When(workingDir.HasDiverged(Any[string]())).ThenReturn(true)
When(workingDir.HasDiverged(Any[logging.SimpleLogging](), Any[string]())).ThenReturn(true)
},
wantFailure: "Default branch must be rebased onto pull request before running plan.",
wantErr: assert.NoError,
Expand Down Expand Up @@ -130,7 +131,7 @@ func TestAggregateApplyRequirements_ValidateApplyProject(t *testing.T) {
ProjectPlanStatus: models.PassedPolicyCheckStatus,
},
setup: func(workingDir *mocks.MockWorkingDir) {
When(workingDir.HasDiverged(Any[string]())).ThenReturn(false)
When(workingDir.HasDiverged(Any[logging.SimpleLogging](), Any[string]())).ThenReturn(false)
},
wantErr: assert.NoError,
},
Expand Down Expand Up @@ -184,7 +185,7 @@ func TestAggregateApplyRequirements_ValidateApplyProject(t *testing.T) {
ApplyRequirements: []string{raw.UnDivergedRequirement},
},
setup: func(workingDir *mocks.MockWorkingDir) {
When(workingDir.HasDiverged(Any[string]())).ThenReturn(true)
When(workingDir.HasDiverged(Any[logging.SimpleLogging](), Any[string]())).ThenReturn(true)
},
wantFailure: "Default branch must be rebased onto pull request before running apply.",
wantErr: assert.NoError,
Expand Down Expand Up @@ -363,7 +364,7 @@ func TestAggregateApplyRequirements_ValidateImportProject(t *testing.T) {
ProjectPlanStatus: models.PassedPolicyCheckStatus,
},
setup: func(workingDir *mocks.MockWorkingDir) {
When(workingDir.HasDiverged(Any[string]())).ThenReturn(false)
When(workingDir.HasDiverged(Any[logging.SimpleLogging](), Any[string]())).ThenReturn(false)
},
wantErr: assert.NoError,
},
Expand Down Expand Up @@ -393,7 +394,7 @@ func TestAggregateApplyRequirements_ValidateImportProject(t *testing.T) {
ImportRequirements: []string{raw.UnDivergedRequirement},
},
setup: func(workingDir *mocks.MockWorkingDir) {
When(workingDir.HasDiverged(Any[string]())).ThenReturn(true)
When(workingDir.HasDiverged(Any[logging.SimpleLogging](), Any[string]())).ThenReturn(true)
},
wantFailure: "Default branch must be rebased onto pull request before running import.",
wantErr: assert.NoError,
Expand Down
85 changes: 54 additions & 31 deletions server/events/command_runner_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -666,12 +666,16 @@ func TestRunUnlockCommand_VCSComment(t *testing.T) {
State: tc.prState,
}
modelPull := models.PullRequest{BaseRepo: testdata.GithubRepo, State: models.OpenPullState, Num: testdata.Pull.Num}
When(githubGetter.GetPullRequest(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(testdata.Pull.Num))).ThenReturn(pull, nil)
When(eventParsing.ParseGithubPull(Any[logging.SimpleLogging](), Eq(pull))).ThenReturn(modelPull, modelPull.BaseRepo, testdata.GithubRepo, nil)
When(githubGetter.GetPullRequest(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo),
Eq(testdata.Pull.Num))).ThenReturn(pull, nil)
When(eventParsing.ParseGithubPull(Any[logging.SimpleLogging](), Eq(pull))).ThenReturn(modelPull, modelPull.BaseRepo,
testdata.GithubRepo, nil)

ch.RunCommentCommand(testdata.GithubRepo, &testdata.GithubRepo, nil, testdata.User, testdata.Pull.Num, &events.CommentCommand{Name: command.Unlock})
ch.RunCommentCommand(testdata.GithubRepo, &testdata.GithubRepo, nil, testdata.User, testdata.Pull.Num,
&events.CommentCommand{Name: command.Unlock})

deleteLockCommand.VerifyWasCalledOnce().DeleteLocksByPull(testdata.GithubRepo.FullName, testdata.Pull.Num)
deleteLockCommand.VerifyWasCalledOnce().DeleteLocksByPull(Any[logging.SimpleLogging](),
Eq(testdata.GithubRepo.FullName), Eq(testdata.Pull.Num))
vcsClient.VerifyWasCalledOnce().CreateComment(
Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(testdata.Pull.Num),
Eq("All Atlantis locks for this PR have been unlocked and plans discarded"), Eq("unlock"))
Expand All @@ -688,11 +692,15 @@ func TestRunUnlockCommandFail_VCSComment(t *testing.T) {
State: github.String("open"),
}
modelPull := models.PullRequest{BaseRepo: testdata.GithubRepo, State: models.OpenPullState, Num: testdata.Pull.Num}
When(githubGetter.GetPullRequest(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(testdata.Pull.Num))).ThenReturn(pull, nil)
When(eventParsing.ParseGithubPull(Any[logging.SimpleLogging](), Eq(pull))).ThenReturn(modelPull, modelPull.BaseRepo, testdata.GithubRepo, nil)
When(deleteLockCommand.DeleteLocksByPull(testdata.GithubRepo.FullName, testdata.Pull.Num)).ThenReturn(0, errors.New("err"))
When(githubGetter.GetPullRequest(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo),
Eq(testdata.Pull.Num))).ThenReturn(pull, nil)
When(eventParsing.ParseGithubPull(Any[logging.SimpleLogging](), Eq(pull))).ThenReturn(modelPull, modelPull.BaseRepo,
testdata.GithubRepo, nil)
When(deleteLockCommand.DeleteLocksByPull(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo.FullName),
Eq(testdata.Pull.Num))).ThenReturn(0, errors.New("err"))

ch.RunCommentCommand(testdata.GithubRepo, &testdata.GithubRepo, nil, testdata.User, testdata.Pull.Num, &events.CommentCommand{Name: command.Unlock})
ch.RunCommentCommand(testdata.GithubRepo, &testdata.GithubRepo, nil, testdata.User, testdata.Pull.Num,
&events.CommentCommand{Name: command.Unlock})

vcsClient.VerifyWasCalledOnce().CreateComment(
Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(testdata.Pull.Num), Eq("Failed to delete PR locks"), Eq("unlock"))
Expand All @@ -708,15 +716,20 @@ func TestRunUnlockCommandFail_DisableUnlockLabel(t *testing.T) {
State: github.String("open"),
}
modelPull := models.PullRequest{BaseRepo: testdata.GithubRepo, State: models.OpenPullState, Num: testdata.Pull.Num}
When(githubGetter.GetPullRequest(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(testdata.Pull.Num))).ThenReturn(pull, nil)
When(eventParsing.ParseGithubPull(Any[logging.SimpleLogging](), Eq(pull))).ThenReturn(modelPull, modelPull.BaseRepo, testdata.GithubRepo, nil)
When(deleteLockCommand.DeleteLocksByPull(testdata.GithubRepo.FullName, testdata.Pull.Num)).ThenReturn(0, errors.New("err"))
When(ch.VCSClient.GetPullLabels(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(modelPull))).ThenReturn([]string{doNotUnlock, "need-help"}, nil)

ch.RunCommentCommand(testdata.GithubRepo, &testdata.GithubRepo, nil, testdata.User, testdata.Pull.Num, &events.CommentCommand{Name: command.Unlock})

vcsClient.VerifyWasCalledOnce().CreateComment(
Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(testdata.Pull.Num), Eq("Not allowed to unlock PR with "+doNotUnlock+" label"), Eq("unlock"))
When(githubGetter.GetPullRequest(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo),
Eq(testdata.Pull.Num))).ThenReturn(pull, nil)
When(eventParsing.ParseGithubPull(Any[logging.SimpleLogging](), Eq(pull))).ThenReturn(modelPull, modelPull.BaseRepo,
testdata.GithubRepo, nil)
When(deleteLockCommand.DeleteLocksByPull(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo.FullName),
Eq(testdata.Pull.Num))).ThenReturn(0, errors.New("err"))
When(ch.VCSClient.GetPullLabels(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo),
Eq(modelPull))).ThenReturn([]string{doNotUnlock, "need-help"}, nil)

ch.RunCommentCommand(testdata.GithubRepo, &testdata.GithubRepo, nil, testdata.User, testdata.Pull.Num,
&events.CommentCommand{Name: command.Unlock})

vcsClient.VerifyWasCalledOnce().CreateComment(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo),
Eq(testdata.Pull.Num), Eq("Not allowed to unlock PR with "+doNotUnlock+" label"), Eq("unlock"))
}

func TestRunUnlockCommandFail_GetLabelsFail(t *testing.T) {
Expand All @@ -727,15 +740,20 @@ func TestRunUnlockCommandFail_GetLabelsFail(t *testing.T) {
State: github.String("open"),
}
modelPull := models.PullRequest{BaseRepo: testdata.GithubRepo, State: models.OpenPullState, Num: testdata.Pull.Num}
When(githubGetter.GetPullRequest(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(testdata.Pull.Num))).ThenReturn(pull, nil)
When(eventParsing.ParseGithubPull(Any[logging.SimpleLogging](), Eq(pull))).ThenReturn(modelPull, modelPull.BaseRepo, testdata.GithubRepo, nil)
When(deleteLockCommand.DeleteLocksByPull(testdata.GithubRepo.FullName, testdata.Pull.Num)).ThenReturn(0, errors.New("err"))
When(ch.VCSClient.GetPullLabels(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(modelPull))).ThenReturn(nil, errors.New("err"))

ch.RunCommentCommand(testdata.GithubRepo, &testdata.GithubRepo, nil, testdata.User, testdata.Pull.Num, &events.CommentCommand{Name: command.Unlock})

vcsClient.VerifyWasCalledOnce().CreateComment(
Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(testdata.Pull.Num), Eq("Failed to retrieve PR labels... Not unlocking"), Eq("unlock"))
When(githubGetter.GetPullRequest(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo),
Eq(testdata.Pull.Num))).ThenReturn(pull, nil)
When(eventParsing.ParseGithubPull(Any[logging.SimpleLogging](), Eq(pull))).ThenReturn(modelPull, modelPull.BaseRepo,
testdata.GithubRepo, nil)
When(deleteLockCommand.DeleteLocksByPull(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo.FullName),
Eq(testdata.Pull.Num))).ThenReturn(0, errors.New("err"))
When(ch.VCSClient.GetPullLabels(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo),
Eq(modelPull))).ThenReturn(nil, errors.New("err"))

ch.RunCommentCommand(testdata.GithubRepo, &testdata.GithubRepo, nil, testdata.User, testdata.Pull.Num,
&events.CommentCommand{Name: command.Unlock})

vcsClient.VerifyWasCalledOnce().CreateComment(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(testdata.Pull.Num),
Eq("Failed to retrieve PR labels... Not unlocking"), Eq("unlock"))
}

func TestRunUnlockCommandDoesntRetrieveLabelsIfDisableUnlockLabelNotSet(t *testing.T) {
Expand All @@ -748,13 +766,18 @@ func TestRunUnlockCommandDoesntRetrieveLabelsIfDisableUnlockLabelNotSet(t *testi
State: github.String("open"),
}
modelPull := models.PullRequest{BaseRepo: testdata.GithubRepo, State: models.OpenPullState, Num: testdata.Pull.Num}
When(githubGetter.GetPullRequest(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(testdata.Pull.Num))).ThenReturn(pull, nil)
When(eventParsing.ParseGithubPull(Any[logging.SimpleLogging](), Eq(pull))).ThenReturn(modelPull, modelPull.BaseRepo, testdata.GithubRepo, nil)
When(deleteLockCommand.DeleteLocksByPull(testdata.GithubRepo.FullName, testdata.Pull.Num)).ThenReturn(0, errors.New("err"))
When(ch.VCSClient.GetPullLabels(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(modelPull))).ThenReturn([]string{doNotUnlock, "need-help"}, nil)
When(githubGetter.GetPullRequest(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo),
Eq(testdata.Pull.Num))).ThenReturn(pull, nil)
When(eventParsing.ParseGithubPull(Any[logging.SimpleLogging](), Eq(pull))).ThenReturn(modelPull, modelPull.BaseRepo,
testdata.GithubRepo, nil)
When(deleteLockCommand.DeleteLocksByPull(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo.FullName),
Eq(testdata.Pull.Num))).ThenReturn(0, errors.New("err"))
When(ch.VCSClient.GetPullLabels(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo),
Eq(modelPull))).ThenReturn([]string{doNotUnlock, "need-help"}, nil)
unlockCommandRunner.DisableUnlockLabel = ""

ch.RunCommentCommand(testdata.GithubRepo, &testdata.GithubRepo, nil, testdata.User, testdata.Pull.Num, &events.CommentCommand{Name: command.Unlock})
ch.RunCommentCommand(testdata.GithubRepo, &testdata.GithubRepo, nil, testdata.User, testdata.Pull.Num,
&events.CommentCommand{Name: command.Unlock})

vcsClient.VerifyWasCalled(Never()).GetPullLabels(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(modelPull))
}
Expand Down
Loading

0 comments on commit 7a9c7a9

Please sign in to comment.