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 logging in the Working Dir Package #4294

Merged
merged 13 commits into from
Mar 27, 2024
Merged

feat: Refine the logging in the Working Dir Package #4294

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
7a9c7a9
Refine the logging in the working dir package
X-Guardian Feb 28, 2024
26ec5b3
Fix tests
X-Guardian Mar 1, 2024
f3eb7d0
Fix tests
X-Guardian Mar 1, 2024
9345c8a
Merge branch 'main' into working-dir-logging-refinement
X-Guardian Mar 1, 2024
4c3d285
Format working_dir_test
X-Guardian Mar 1, 2024
68ac355
Merge branch 'working-dir-logging-refinement' of https://github.com/X…
X-Guardian Mar 1, 2024
3a7ff57
Remove unused //nolint
X-Guardian Mar 1, 2024
86a0405
Merge branch 'main' into working-dir-logging-refinement
X-Guardian Mar 11, 2024
b82e8ed
Merge branch 'main' into working-dir-logging-refinement
X-Guardian Mar 15, 2024
c0b1fff
Merge branch 'main' into working-dir-logging-refinement
X-Guardian Mar 24, 2024
f256193
Fix formatting
X-Guardian Mar 24, 2024
5430ae7
Fix formatting
X-Guardian Mar 24, 2024
a31edc5
Merge branch 'main' into working-dir-logging-refinement
GenPage Mar 27, 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: 0 additions & 2 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 @@ -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
View file
Open in desktop
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
26 changes: 13 additions & 13 deletions server/controllers/locks_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ func TestGetLock_None(t *testing.T) {
req = mux.SetURLVars(req, map[string]string{"id": "id"})
w := httptest.NewRecorder()
lc.GetLock(w, req)
ResponseContains(t, w, http.StatusNotFound, "No lock found at id \"id\"")
ResponseContains(t, w, http.StatusNotFound, "No lock found at id 'id'")
}

func TestGetLock_Success(t *testing.T) {
Expand Down Expand Up @@ -215,14 +215,14 @@ func TestDeleteLock_InvalidLockID(t *testing.T) {
req = mux.SetURLVars(req, map[string]string{"id": "%A@"})
w := httptest.NewRecorder()
lc.DeleteLock(w, req)
ResponseContains(t, w, http.StatusBadRequest, "Invalid lock id \"%A@\"")
ResponseContains(t, w, http.StatusBadRequest, "Invalid lock id '%A@'")
}

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 @@ -247,15 +247,15 @@ func TestDeleteLock_None(t *testing.T) {
req = mux.SetURLVars(req, map[string]string{"id": "id"})
w := httptest.NewRecorder()
lc.DeleteLock(w, req)
ResponseContains(t, w, http.StatusNotFound, "No lock found at id \"id\"")
ResponseContains(t, w, http.StatusNotFound, "No lock found at id 'id'")
}

func TestDeleteLock_OldFormat(t *testing.T) {
t.Log("If the lock doesn't have BaseRepo set it is deleted successfully")
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 All @@ -265,7 +265,7 @@ func TestDeleteLock_OldFormat(t *testing.T) {
req = mux.SetURLVars(req, map[string]string{"id": "id"})
w := httptest.NewRecorder()
lc.DeleteLock(w, req)
ResponseContains(t, w, http.StatusOK, "Deleted lock id \"id\"")
ResponseContains(t, w, http.StatusOK, "Deleted lock id 'id'")
cp.VerifyWasCalled(Never()).CreateComment(Any[logging.SimpleLogging](), Any[models.Repo](), Any[int](), Any[string](), Any[string]())
}

Expand All @@ -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 @@ -321,7 +321,7 @@ func TestDeleteLock_UpdateProjectStatus(t *testing.T) {
req = mux.SetURLVars(req, map[string]string{"id": "id"})
w := httptest.NewRecorder()
lc.DeleteLock(w, req)
ResponseContains(t, w, http.StatusOK, "Deleted lock id \"id\"")
ResponseContains(t, w, http.StatusOK, "Deleted lock id 'id'")
status, err := backend.GetPullStatus(pull)
Ok(t, err)
Assert(t, status.Projects != nil, "status projects was nil")
Expand All @@ -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 All @@ -363,7 +363,7 @@ func TestDeleteLock_CommentFailed(t *testing.T) {
req = mux.SetURLVars(req, map[string]string{"id": "id"})
w := httptest.NewRecorder()
lc.DeleteLock(w, req)
ResponseContains(t, w, http.StatusOK, "Deleted lock id \"id\"")
ResponseContains(t, w, http.StatusOK, "Deleted lock id 'id'")
}

func TestDeleteLock_CommentSuccess(t *testing.T) {
Expand All @@ -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 All @@ -400,7 +400,7 @@ func TestDeleteLock_CommentSuccess(t *testing.T) {
req = mux.SetURLVars(req, map[string]string{"id": "id"})
w := httptest.NewRecorder()
lc.DeleteLock(w, req)
ResponseContains(t, w, http.StatusOK, "Deleted lock id \"id\"")
ResponseContains(t, w, http.StatusOK, "Deleted lock id 'id'")
cp.VerifyWasCalled(Once()).CreateComment(Any[logging.SimpleLogging](), Eq(pull.BaseRepo), Eq(pull.Num),
Eq("**Warning**: The plan for dir: `path` workspace: `workspace` was **discarded** via the Atlantis UI.\n\n"+
"To `apply` this plan you must run `plan` again."), Eq(""))
Expand Down
6 changes: 3 additions & 3 deletions server/events/command_requirement_handler.go
View file
Open in desktop
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
View file
Open in desktop
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
Loading
Loading