FIx shared structs + add better debugging/linting (#379)

* fix: Fixes issues w/ shared pointers to structs (#378) * feat: Adds even better debugging and linting support (#376) This is a #PATCH release.
harrisoncramer · Sep 16, 2024 · 5c9b88d · 5c9b88d
1 parent c3d7f26
commit 5c9b88d
Show file tree

Hide file tree

Showing 31 changed files with 191 additions and 155 deletions.
diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md
@@ -31,11 +31,12 @@ If you are making changes to the Go codebase, don't forget to run `make compile`
 
 3. Apply formatters and linters to your changes
 
-For changes to the Go codebase: We use <a href="https://pkg.go.dev/cmd/gofmt">gofmt</a> to check formatting and <a href="https://github.com/golangci/golangci-lint">golangci-lint</a> to check linting. Run these commands in the root of the repository:
+For changes to the Go codebase: We use <a href="https://pkg.go.dev/cmd/gofmt">gofmt</a> to check formatting and <a href="https://github.com/golangci/golangci-lint">golangci-lint</a> to check linting, and <a href="https://staticcheck.dev/">staticcheck</a>. Run these commands in the root of the repository:
 
 ```bash
 $ go fmt ./...
-$ golangci-lint run
+$ golangci-lint run ./...
+$ staticcheck ./...
 ```
 
 If you are writing tests and have added something to the Go client, you can test with:

diff --git a/.github/workflows/go.yaml b/.github/workflows/go.yaml
@@ -12,14 +12,18 @@ jobs:
       - uses: actions/checkout@v3
       - uses: actions/setup-go@v4
         with:
-          go-version: '1.19'
+          go-version: '1.23.1'
           cache: false
       - name: golangci-lint
         uses: golangci/golangci-lint-action@v3
         with:
-          version: v1.54
+          version: v1.61.0
           only-new-issues: true
           skip-cache: true
+      - name: Install staticcheck
+        run: go install honnef.co/go/tools/cmd/[email protected]
+      - name: Run staticcheck
+        run: staticcheck ./...
   go_test:
     name: Test Go 🧪 
     needs: [go_lint]

diff --git a/.golangci.yml b/.golangci.yml
@@ -0,0 +1,3 @@
+run:
+  tests: true
+  timeout: 30s
diff --git a/cmd/app/assignee.go b/cmd/app/assignee.go
@@ -28,7 +28,7 @@ func (a assigneesService) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	assigneeUpdateRequest, ok := r.Context().Value(payload("payload")).(*AssigneeUpdateRequest)
 
 	if !ok {
-		handleError(w, errors.New("Could not get payload from context"), "Bad payload", http.StatusInternalServerError)
+		handleError(w, errors.New("could not get payload from context"), "Bad payload", http.StatusInternalServerError)
 		return
 	}
 

diff --git a/cmd/app/assignee_test.go b/cmd/app/assignee_test.go
@@ -27,7 +27,7 @@ func TestAssigneeHandler(t *testing.T) {
 		svc := middleware(
 			assigneesService{testProjectData, fakeAssigneeClient{}},
 			withMr(testProjectData, fakeMergeRequestLister{}),
-			withPayloadValidation(methodToPayload{http.MethodPut: &AssigneeUpdateRequest{}}),
+			withPayloadValidation(methodToPayload{http.MethodPut: newPayload[AssigneeUpdateRequest]}),
 			withMethodCheck(http.MethodPut),
 		)
 		data := getSuccessData(t, svc, request)
@@ -40,7 +40,7 @@ func TestAssigneeHandler(t *testing.T) {
 		svc := middleware(
 			assigneesService{testProjectData, client},
 			withMr(testProjectData, fakeMergeRequestLister{}),
-			withPayloadValidation(methodToPayload{http.MethodPut: &AssigneeUpdateRequest{}}),
+			withPayloadValidation(methodToPayload{http.MethodPut: newPayload[AssigneeUpdateRequest]}),
 			withMethodCheck(http.MethodPut),
 		)
 		data, _ := getFailData(t, svc, request)
@@ -53,7 +53,7 @@ func TestAssigneeHandler(t *testing.T) {
 		svc := middleware(
 			assigneesService{testProjectData, client},
 			withMr(testProjectData, fakeMergeRequestLister{}),
-			withPayloadValidation(methodToPayload{http.MethodPut: &AssigneeUpdateRequest{}}),
+			withPayloadValidation(methodToPayload{http.MethodPut: newPayload[AssigneeUpdateRequest]}),
 			withMethodCheck(http.MethodPut),
 		)
 		data, _ := getFailData(t, svc, request)

diff --git a/cmd/app/attachment_test.go b/cmd/app/attachment_test.go
@@ -38,7 +38,7 @@ func TestAttachmentHandler(t *testing.T) {
 		request := makeRequest(t, http.MethodPost, "/attachment", attachmentTestRequestData)
 		svc := middleware(
 			attachmentService{testProjectData, fakeFileReader{}, fakeFileUploaderClient{}},
-			withPayloadValidation(methodToPayload{http.MethodPost: &AttachmentRequest{}}),
+			withPayloadValidation(methodToPayload{http.MethodPost: newPayload[AttachmentRequest]}),
 			withMethodCheck(http.MethodPost),
 		)
 		data := getSuccessData(t, svc, request)
@@ -49,7 +49,7 @@ func TestAttachmentHandler(t *testing.T) {
 		request := makeRequest(t, http.MethodPost, "/attachment", attachmentTestRequestData)
 		svc := middleware(
 			attachmentService{testProjectData, fakeFileReader{}, fakeFileUploaderClient{testBase{errFromGitlab: true}}},
-			withPayloadValidation(methodToPayload{http.MethodPost: &AttachmentRequest{}}),
+			withPayloadValidation(methodToPayload{http.MethodPost: newPayload[AttachmentRequest]}),
 			withMethodCheck(http.MethodPost),
 		)
 		data, _ := getFailData(t, svc, request)
@@ -60,7 +60,7 @@ func TestAttachmentHandler(t *testing.T) {
 		request := makeRequest(t, http.MethodPost, "/attachment", attachmentTestRequestData)
 		svc := middleware(
 			attachmentService{testProjectData, fakeFileReader{}, fakeFileUploaderClient{testBase{status: http.StatusSeeOther}}},
-			withPayloadValidation(methodToPayload{http.MethodPost: &AttachmentRequest{}}),
+			withPayloadValidation(methodToPayload{http.MethodPost: newPayload[AttachmentRequest]}),
 			withMethodCheck(http.MethodPost),
 		)
 		data, _ := getFailData(t, svc, request)

diff --git a/cmd/app/client.go b/cmd/app/client.go
@@ -33,13 +33,13 @@ type Client struct {
 }
 
 /* NewClient parses and validates the project settings and initializes the Gitlab client. */
-func NewClient() (error, *Client) {
+func NewClient() (*Client, error) {
 
 	if pluginOptions.GitlabUrl == "" {
-		return errors.New("GitLab instance URL cannot be empty"), nil
+		return nil, errors.New("GitLab instance URL cannot be empty")
 	}
 
-	var apiCustUrl = fmt.Sprintf(pluginOptions.GitlabUrl + "/api/v4")
+	var apiCustUrl = fmt.Sprintf("%s/api/v4", pluginOptions.GitlabUrl)
 
 	gitlabOptions := []gitlab.ClientOptionFunc{
 		gitlab.WithBaseURL(apiCustUrl),
@@ -73,10 +73,10 @@ func NewClient() (error, *Client) {
 	client, err := gitlab.NewClient(pluginOptions.AuthToken, gitlabOptions...)
 
 	if err != nil {
-		return fmt.Errorf("Failed to create client: %v", err), nil
+		return nil, fmt.Errorf("failed to create client: %v", err)
 	}
 
-	return nil, &Client{
+	return &Client{
 		MergeRequestsService:         client.MergeRequests,
 		MergeRequestApprovalsService: client.MergeRequestApprovals,
 		DiscussionsService:           client.Discussions,
@@ -88,28 +88,28 @@ func NewClient() (error, *Client) {
 		AwardEmojiService:            client.AwardEmoji,
 		UsersService:                 client.Users,
 		DraftNotesService:            client.DraftNotes,
-	}
+	}, nil
 }
 
 /* InitProjectSettings fetch the project ID using the client */
-func InitProjectSettings(c *Client, gitInfo git.GitData) (error, *ProjectInfo) {
+func InitProjectSettings(c *Client, gitInfo git.GitData) (*ProjectInfo, error) {
 
 	opt := gitlab.GetProjectOptions{}
 	project, _, err := c.GetProject(gitInfo.ProjectPath(), &opt)
 
 	if err != nil {
-		return fmt.Errorf(fmt.Sprintf("Error getting project at %s", gitInfo.RemoteUrl), err), nil
+		return nil, fmt.Errorf(fmt.Sprintf("Error getting project at %s", gitInfo.RemoteUrl), err)
 	}
 
 	if project == nil {
-		return fmt.Errorf(fmt.Sprintf("Could not find project at %s", gitInfo.RemoteUrl), err), nil
+		return nil, fmt.Errorf(fmt.Sprintf("Could not find project at %s", gitInfo.RemoteUrl), err)
 	}
 
 	projectId := fmt.Sprint(project.ID)
 
-	return nil, &ProjectInfo{
+	return &ProjectInfo{
 		ProjectId: projectId,
-	}
+	}, nil
 }
 
 /* handleError is a utililty handler that returns errors to the client along with their statuses and messages */

diff --git a/cmd/app/comment_test.go b/cmd/app/comment_test.go
@@ -44,9 +44,9 @@ func TestPostComment(t *testing.T) {
 			commentService{testProjectData, fakeCommentClient{}},
 			withMr(testProjectData, fakeMergeRequestLister{}),
 			withPayloadValidation(methodToPayload{
-				http.MethodPost:   &PostCommentRequest{},
-				http.MethodDelete: &DeleteCommentRequest{},
-				http.MethodPatch:  &EditCommentRequest{},
+				http.MethodPost:   newPayload[PostCommentRequest],
+				http.MethodDelete: newPayload[DeleteCommentRequest],
+				http.MethodPatch:  newPayload[EditCommentRequest],
 			}),
 			withMethodCheck(http.MethodPost, http.MethodDelete, http.MethodPatch),
 		)
@@ -66,9 +66,9 @@ func TestPostComment(t *testing.T) {
 			commentService{testProjectData, fakeCommentClient{}},
 			withMr(testProjectData, fakeMergeRequestLister{}),
 			withPayloadValidation(methodToPayload{
-				http.MethodPost:   &PostCommentRequest{},
-				http.MethodDelete: &DeleteCommentRequest{},
-				http.MethodPatch:  &EditCommentRequest{},
+				http.MethodPost:   newPayload[PostCommentRequest],
+				http.MethodDelete: newPayload[DeleteCommentRequest],
+				http.MethodPatch:  newPayload[EditCommentRequest],
 			}),
 			withMethodCheck(http.MethodPost, http.MethodDelete, http.MethodPatch),
 		)
@@ -82,9 +82,9 @@ func TestPostComment(t *testing.T) {
 			commentService{testProjectData, fakeCommentClient{testBase{errFromGitlab: true}}},
 			withMr(testProjectData, fakeMergeRequestLister{}),
 			withPayloadValidation(methodToPayload{
-				http.MethodPost:   &PostCommentRequest{},
-				http.MethodDelete: &DeleteCommentRequest{},
-				http.MethodPatch:  &EditCommentRequest{},
+				http.MethodPost:   newPayload[PostCommentRequest],
+				http.MethodDelete: newPayload[DeleteCommentRequest],
+				http.MethodPatch:  newPayload[EditCommentRequest],
 			}),
 			withMethodCheck(http.MethodPost, http.MethodDelete, http.MethodPatch),
 		)
@@ -98,9 +98,9 @@ func TestPostComment(t *testing.T) {
 			commentService{testProjectData, fakeCommentClient{testBase{status: http.StatusSeeOther}}},
 			withMr(testProjectData, fakeMergeRequestLister{}),
 			withPayloadValidation(methodToPayload{
-				http.MethodPost:   &PostCommentRequest{},
-				http.MethodDelete: &DeleteCommentRequest{},
-				http.MethodPatch:  &EditCommentRequest{},
+				http.MethodPost:   newPayload[PostCommentRequest],
+				http.MethodDelete: newPayload[DeleteCommentRequest],
+				http.MethodPatch:  newPayload[EditCommentRequest],
 			}),
 			withMethodCheck(http.MethodPost, http.MethodDelete, http.MethodPatch),
 		)
@@ -117,9 +117,9 @@ func TestDeleteComment(t *testing.T) {
 			commentService{testProjectData, fakeCommentClient{}},
 			withMr(testProjectData, fakeMergeRequestLister{}),
 			withPayloadValidation(methodToPayload{
-				http.MethodPost:   &PostCommentRequest{},
-				http.MethodDelete: &DeleteCommentRequest{},
-				http.MethodPatch:  &EditCommentRequest{},
+				http.MethodPost:   newPayload[PostCommentRequest],
+				http.MethodDelete: newPayload[DeleteCommentRequest],
+				http.MethodPatch:  newPayload[EditCommentRequest],
 			}),
 			withMethodCheck(http.MethodPost, http.MethodDelete, http.MethodPatch),
 		)
@@ -136,9 +136,9 @@ func TestEditComment(t *testing.T) {
 			commentService{testProjectData, fakeCommentClient{}},
 			withMr(testProjectData, fakeMergeRequestLister{}),
 			withPayloadValidation(methodToPayload{
-				http.MethodPost:   &PostCommentRequest{},
-				http.MethodDelete: &DeleteCommentRequest{},
-				http.MethodPatch:  &EditCommentRequest{},
+				http.MethodPost:   newPayload[PostCommentRequest],
+				http.MethodDelete: newPayload[DeleteCommentRequest],
+				http.MethodPatch:  newPayload[EditCommentRequest],
 			}),
 			withMethodCheck(http.MethodPost, http.MethodDelete, http.MethodPatch),
 		)

diff --git a/cmd/app/create_mr_test.go b/cmd/app/create_mr_test.go
@@ -31,7 +31,7 @@ func TestCreateMr(t *testing.T) {
 		request := makeRequest(t, http.MethodPost, "/create_mr", testCreateMrRequestData)
 		svc := middleware(
 			mergeRequestCreatorService{testProjectData, fakeMergeCreatorClient{}},
-			withPayloadValidation(methodToPayload{http.MethodPost: &CreateMrRequest{}}),
+			withPayloadValidation(methodToPayload{http.MethodPost: newPayload[CreateMrRequest]}),
 			withMethodCheck(http.MethodPost),
 		)
 		data := getSuccessData(t, svc, request)
@@ -42,7 +42,7 @@ func TestCreateMr(t *testing.T) {
 		request := makeRequest(t, http.MethodPost, "/create_mr", testCreateMrRequestData)
 		svc := middleware(
 			mergeRequestCreatorService{testProjectData, fakeMergeCreatorClient{testBase{errFromGitlab: true}}},
-			withPayloadValidation(methodToPayload{http.MethodPost: &CreateMrRequest{}}),
+			withPayloadValidation(methodToPayload{http.MethodPost: newPayload[CreateMrRequest]}),
 			withMethodCheck(http.MethodPost),
 		)
 		data, _ := getFailData(t, svc, request)
@@ -53,7 +53,7 @@ func TestCreateMr(t *testing.T) {
 		request := makeRequest(t, http.MethodPost, "/create_mr", testCreateMrRequestData)
 		svc := middleware(
 			mergeRequestCreatorService{testProjectData, fakeMergeCreatorClient{testBase{status: http.StatusSeeOther}}},
-			withPayloadValidation(methodToPayload{http.MethodPost: &CreateMrRequest{}}),
+			withPayloadValidation(methodToPayload{http.MethodPost: newPayload[CreateMrRequest]}),
 			withMethodCheck(http.MethodPost),
 		)
 		data, _ := getFailData(t, svc, request)
@@ -66,7 +66,7 @@ func TestCreateMr(t *testing.T) {
 		request := makeRequest(t, http.MethodPost, "/create_mr", reqData)
 		svc := middleware(
 			mergeRequestCreatorService{testProjectData, fakeMergeCreatorClient{}},
-			withPayloadValidation(methodToPayload{http.MethodPost: &CreateMrRequest{}}),
+			withPayloadValidation(methodToPayload{http.MethodPost: newPayload[CreateMrRequest]}),
 			withMethodCheck(http.MethodPost),
 		)
 		data, _ := getFailData(t, svc, request)
@@ -80,7 +80,7 @@ func TestCreateMr(t *testing.T) {
 		request := makeRequest(t, http.MethodPost, "/create_mr", reqData)
 		svc := middleware(
 			mergeRequestCreatorService{testProjectData, fakeMergeCreatorClient{}},
-			withPayloadValidation(methodToPayload{http.MethodPost: &CreateMrRequest{}}),
+			withPayloadValidation(methodToPayload{http.MethodPost: newPayload[CreateMrRequest]}),
 			withMethodCheck(http.MethodPost),
 		)
 		data, _ := getFailData(t, svc, request)

diff --git a/cmd/app/draft_note_publisher_test.go b/cmd/app/draft_note_publisher_test.go
@@ -25,7 +25,7 @@ func TestPublishDraftNote(t *testing.T) {
 		svc := middleware(
 			draftNotePublisherService{testProjectData, fakeDraftNotePublisher{}},
 			withMr(testProjectData, fakeMergeRequestLister{}),
-			withPayloadValidation(methodToPayload{http.MethodPost: &DraftNotePublishRequest{}}),
+			withPayloadValidation(methodToPayload{http.MethodPost: newPayload[DraftNotePublishRequest]}),
 			withMethodCheck(http.MethodPost),
 		)
 		data := getSuccessData(t, svc, request)
@@ -36,7 +36,7 @@ func TestPublishDraftNote(t *testing.T) {
 		svc := middleware(
 			draftNotePublisherService{testProjectData, fakeDraftNotePublisher{testBase: testBase{errFromGitlab: true}}},
 			withMr(testProjectData, fakeMergeRequestLister{}),
-			withPayloadValidation(methodToPayload{http.MethodPost: &DraftNotePublishRequest{}}),
+			withPayloadValidation(methodToPayload{http.MethodPost: newPayload[DraftNotePublishRequest]}),
 			withMethodCheck(http.MethodPost),
 		)
 		data, _ := getFailData(t, svc, request)
@@ -51,7 +51,7 @@ func TestPublishAllDraftNotes(t *testing.T) {
 		svc := middleware(
 			draftNotePublisherService{testProjectData, fakeDraftNotePublisher{}},
 			withMr(testProjectData, fakeMergeRequestLister{}),
-			withPayloadValidation(methodToPayload{http.MethodPost: &DraftNotePublishRequest{}}),
+			withPayloadValidation(methodToPayload{http.MethodPost: newPayload[DraftNotePublishRequest]}),
 			withMethodCheck(http.MethodPost),
 		)
 		data := getSuccessData(t, svc, request)
@@ -62,7 +62,7 @@ func TestPublishAllDraftNotes(t *testing.T) {
 		svc := middleware(
 			draftNotePublisherService{testProjectData, fakeDraftNotePublisher{testBase: testBase{errFromGitlab: true}}},
 			withMr(testProjectData, fakeMergeRequestLister{}),
-			withPayloadValidation(methodToPayload{http.MethodPost: &DraftNotePublishRequest{}}),
+			withPayloadValidation(methodToPayload{http.MethodPost: newPayload[DraftNotePublishRequest]}),
 			withMethodCheck(http.MethodPost),
 		)
 		data, _ := getFailData(t, svc, request)

diff --git a/cmd/app/draft_notes.go b/cmd/app/draft_notes.go
@@ -183,7 +183,7 @@ func (a draftNoteService) updateDraftNote(w http.ResponseWriter, r *http.Request
 	payload := r.Context().Value(payload("payload")).(*UpdateDraftNoteRequest)
 
 	if payload.Note == "" {
-		handleError(w, errors.New("Draft note text missing"), "Must provide draft note text", http.StatusBadRequest)
+		handleError(w, errors.New("draft note text missing"), "Must provide draft note text", http.StatusBadRequest)
 		return
 	}