diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md index 0a07071c..98d3c074 100644 --- 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 gofmt to check formatting and golangci-lint to check linting. Run these commands in the root of the repository: +For changes to the Go codebase: We use gofmt to check formatting and golangci-lint to check linting, and staticcheck. 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 index 01e1dbee..2786b74b 100644 --- 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/staticcheck@2024.1.1 + - name: Run staticcheck + run: staticcheck ./... go_test: name: Test Go 🧪 needs: [go_lint] diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 00000000..2ba75e1f --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,3 @@ +run: + tests: true + timeout: 30s diff --git a/cmd/app/assignee.go b/cmd/app/assignee.go index b7be1670..17fb1b66 100644 --- 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/client.go b/cmd/app/client.go index 756371f7..4d3da2ed 100644 --- 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/draft_notes.go b/cmd/app/draft_notes.go index 6c248115..42daaa39 100644 --- 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 } diff --git a/cmd/app/emoji.go b/cmd/app/emoji.go index 4a535038..2dbfe6fb 100644 --- a/cmd/app/emoji.go +++ b/cmd/app/emoji.go @@ -64,6 +64,11 @@ func (a emojiService) deleteEmojiFromNote(w http.ResponseWriter, r *http.Request suffix := strings.TrimPrefix(r.URL.Path, "/mr/awardable/note/") ids := strings.Split(suffix, "/") + if len(ids) < 2 { + handleError(w, errors.New("missing IDs"), "Must provide note ID and awardable ID", http.StatusBadRequest) + return + } + noteId, err := strconv.Atoi(ids[0]) if err != nil { handleError(w, err, "Could not convert note ID to integer", http.StatusBadRequest) @@ -158,18 +163,18 @@ func attachEmojis(a *data, fr FileReader) error { reader, err := fr.ReadFile(filePath) if err != nil { - return fmt.Errorf("Could not find emojis at %s", filePath) + return fmt.Errorf("could not find emojis at %s", filePath) } bytes, err := io.ReadAll(reader) if err != nil { - return errors.New("Could not read emoji file") + return errors.New("could not read emoji file") } var emojiMap EmojiMap err = json.Unmarshal(bytes, &emojiMap) if err != nil { - return errors.New("Could not unmarshal emojis") + return errors.New("could not unmarshal emojis") } a.emojiMap = emojiMap diff --git a/cmd/app/git/git.go b/cmd/app/git/git.go index 7307430b..b4e36f87 100644 --- a/cmd/app/git/git.go +++ b/cmd/app/git/git.go @@ -39,12 +39,12 @@ Gitlab project and the branch must be a feature branch func NewGitData(remote string, g GitManager) (GitData, error) { err := g.RefreshProjectInfo(remote) if err != nil { - return GitData{}, fmt.Errorf("Could not get latest information from remote: %v", err) + return GitData{}, fmt.Errorf("could not get latest information from remote: %v", err) } url, err := g.GetProjectUrlFromNativeGitCmd(remote) if err != nil { - return GitData{}, fmt.Errorf("Could not get project Url: %v", err) + return GitData{}, fmt.Errorf("could not get project Url: %v", err) } /* @@ -62,7 +62,7 @@ func NewGitData(remote string, g GitManager) (GitData, error) { re := regexp.MustCompile(`^(?:git@[^\/:]*|https?:\/\/[^\/]+|ssh:\/\/[^\/:]+)(?::\d+)?[\/:](.*)\/([^\/]+?)(?:\.git)?\/?$`) matches := re.FindStringSubmatch(url) if len(matches) != 3 { - return GitData{}, fmt.Errorf("Invalid Git URL format: %s", url) + return GitData{}, fmt.Errorf("invalid git URL format: %s", url) } namespace := matches[1] @@ -70,7 +70,7 @@ func NewGitData(remote string, g GitManager) (GitData, error) { branchName, err := g.GetCurrentBranchNameFromNativeGitCmd() if err != nil { - return GitData{}, fmt.Errorf("Failed to get current branch: %v", err) + return GitData{}, fmt.Errorf("failed to get current branch: %v", err) } return GitData{ @@ -88,7 +88,7 @@ func (g Git) GetCurrentBranchNameFromNativeGitCmd() (res string, e error) { output, err := gitCmd.Output() if err != nil { - return "", fmt.Errorf("Error running git rev-parse: %w", err) + return "", fmt.Errorf("error running git rev-parse: %w", err) } branchName := strings.TrimSpace(string(output)) @@ -101,7 +101,7 @@ func (g Git) GetProjectUrlFromNativeGitCmd(remote string) (string, error) { cmd := exec.Command("git", "remote", "get-url", remote) url, err := cmd.Output() if err != nil { - return "", fmt.Errorf("Could not get remote") + return "", fmt.Errorf("could not get remote") } return strings.TrimSpace(string(url)), nil @@ -112,7 +112,7 @@ func (g Git) RefreshProjectInfo(remote string) error { cmd := exec.Command("git", "fetch", remote) _, err := cmd.Output() if err != nil { - return fmt.Errorf("Failed to run `git fetch %s`: %v", remote, err) + return fmt.Errorf("failed to run `git fetch %s`: %v", remote, err) } return nil @@ -123,7 +123,7 @@ func (g Git) GetLatestCommitOnRemote(remote string, branchName string) (string, out, err := cmd.Output() if err != nil { - return "", fmt.Errorf("Failed to run `git log -1 --format=%%H " + fmt.Sprintf("%s/%s", remote, branchName)) + return "", fmt.Errorf("failed to run `git log -1 --format=%%H %s/%s`", remote, branchName) } commit := strings.TrimSpace(string(out)) diff --git a/cmd/app/list_discussions.go b/cmd/app/list_discussions.go index a75134ce..555a151e 100644 --- a/cmd/app/list_discussions.go +++ b/cmd/app/list_discussions.go @@ -87,7 +87,7 @@ func (a discussionsListerService) ServeHTTP(w http.ResponseWriter, r *http.Reque var linkedDiscussions []*gitlab.Discussion for _, discussion := range discussions { - if discussion.Notes == nil || len(discussion.Notes) == 0 || Contains(request.Blacklist, discussion.Notes[0].Author.Username) { + if len(discussion.Notes) == 0 || Contains(request.Blacklist, discussion.Notes[0].Author.Username) { continue } for _, note := range discussion.Notes { diff --git a/cmd/app/logging.go b/cmd/app/logging.go index e7fda5d9..d85820db 100644 --- a/cmd/app/logging.go +++ b/cmd/app/logging.go @@ -47,7 +47,7 @@ func (l LoggingServer) ServeHTTP(w http.ResponseWriter, r *http.Request) { Request: r, } if pluginOptions.Debug.Response { - logResponse("RESPONSE FROM GO SERVER", resp) + logResponse("RESPONSE FROM GO SERVER", resp) //nolint:errcheck } } @@ -62,7 +62,7 @@ func logRequest(prefix string, r *http.Request) { os.Exit(1) } r.Header.Set("Private-Token", token) - _, err = file.Write([]byte(fmt.Sprintf("\n-- %s --\n%s\n", prefix, res))) //nolint:all + fmt.Fprintf(file, "\n-- %s --\n%s\n", prefix, res) //nolint:errcheck } func logResponse(prefix string, r *http.Response) { @@ -75,7 +75,7 @@ func logResponse(prefix string, r *http.Response) { os.Exit(1) } - _, err = file.Write([]byte(fmt.Sprintf("\n-- %s --\n%s\n", prefix, res))) //nolint:all + fmt.Fprintf(file, "\n-- %s --\n%s\n", prefix, res) //nolint:errcheck } func openLogFile() *os.File { diff --git a/cmd/app/merge_requests.go b/cmd/app/merge_requests.go index 4fd58b6d..7f1510e7 100644 --- a/cmd/app/merge_requests.go +++ b/cmd/app/merge_requests.go @@ -48,7 +48,7 @@ func (a mergeRequestListerService) ServeHTTP(w http.ResponseWriter, r *http.Requ } if len(mergeRequests) == 0 { - handleError(w, errors.New("No merge requests found"), "No merge requests found", http.StatusNotFound) + handleError(w, errors.New("no merge requests found"), "No merge requests found", http.StatusNotFound) return } @@ -60,6 +60,6 @@ func (a mergeRequestListerService) ServeHTTP(w http.ResponseWriter, r *http.Requ err = json.NewEncoder(w).Encode(response) if err != nil { - handleError(w, err, "Could not encode response", http.StatusInternalServerError) + handleError(w, err, "could not encode response", http.StatusInternalServerError) } } diff --git a/cmd/app/merge_requests_by_username_test.go b/cmd/app/merge_requests_by_username_test.go index a6c2d010..69923e38 100644 --- a/cmd/app/merge_requests_by_username_test.go +++ b/cmd/app/merge_requests_by_username_test.go @@ -91,7 +91,7 @@ func TestListMergeRequestByUsername(t *testing.T) { ) data, status := getFailData(t, svc, request) assert(t, data.Message, "An error occurred") - assert(t, data.Details, strings.Repeat("Some error from Gitlab; ", 3)) + assert(t, data.Details, strings.Repeat("some error from Gitlab; ", 3)) assert(t, status, http.StatusInternalServerError) }) diff --git a/cmd/app/middleware.go b/cmd/app/middleware.go index ae991911..2725c243 100644 --- a/cmd/app/middleware.go +++ b/cmd/app/middleware.go @@ -101,18 +101,18 @@ func (m withMrMiddleware) handle(next http.Handler) http.Handler { mergeRequests, _, err := m.client.ListProjectMergeRequests(m.data.projectInfo.ProjectId, &options) if err != nil { - handleError(w, fmt.Errorf("Failed to list merge requests: %w", err), "Failed to list merge requests", http.StatusInternalServerError) + handleError(w, fmt.Errorf("failed to list merge requests: %w", err), "Failed to list merge requests", http.StatusInternalServerError) return } if len(mergeRequests) == 0 { - err := fmt.Errorf("Branch '%s' does not have any merge requests", m.data.gitInfo.BranchName) + err := fmt.Errorf("branch '%s' does not have any merge requests", m.data.gitInfo.BranchName) handleError(w, err, "No MRs Found", http.StatusNotFound) return } if len(mergeRequests) > 1 { - err := errors.New("Please call gitlab.choose_merge_request()") + err := errors.New("please call gitlab.choose_merge_request()") handleError(w, err, "Multiple MRs found", http.StatusBadRequest) return } @@ -155,9 +155,9 @@ func withMethodCheck(methods ...string) mw { } // Helper function to format validation errors into more readable strings -func formatValidationErrors(errors validator.ValidationErrors) error { +func formatValidationErrors(errs validator.ValidationErrors) error { var s strings.Builder - for i, e := range errors { + for i, e := range errs { if i > 0 { s.WriteString("; ") } @@ -169,5 +169,5 @@ func formatValidationErrors(errors validator.ValidationErrors) error { } } - return fmt.Errorf(s.String()) + return errors.New(s.String()) } diff --git a/cmd/app/middleware_test.go b/cmd/app/middleware_test.go index 6e9afdfb..8a480928 100644 --- a/cmd/app/middleware_test.go +++ b/cmd/app/middleware_test.go @@ -75,7 +75,7 @@ func TestWithMrMiddleware(t *testing.T) { data, status := getFailData(t, handler, request) assert(t, status, http.StatusNotFound) assert(t, data.Message, "No MRs Found") - assert(t, data.Details, "Branch 'foo' does not have any merge requests") + assert(t, data.Details, "branch 'foo' does not have any merge requests") }) t.Run("Handles when there are too many MRs", func(t *testing.T) { request := makeRequest(t, http.MethodGet, "/foo", nil) @@ -88,7 +88,7 @@ func TestWithMrMiddleware(t *testing.T) { data, status := getFailData(t, handler, request) assert(t, status, http.StatusBadRequest) assert(t, data.Message, "Multiple MRs found") - assert(t, data.Details, "Please call gitlab.choose_merge_request()") + assert(t, data.Details, "please call gitlab.choose_merge_request()") }) } diff --git a/cmd/app/pipeline.go b/cmd/app/pipeline.go index 8640613a..174b4acc 100644 --- a/cmd/app/pipeline.go +++ b/cmd/app/pipeline.go @@ -67,7 +67,7 @@ func (a pipelineService) GetLastPipeline(commit string) (*gitlab.PipelineInfo, e } if res.StatusCode >= 300 { - return nil, errors.New("Could not get pipelines") + return nil, errors.New("could not get pipelines") } if len(pipes) == 0 { diff --git a/cmd/app/resolve_discussion_test.go b/cmd/app/resolve_discussion_test.go index 18918e1a..7396b1a0 100644 --- a/cmd/app/resolve_discussion_test.go +++ b/cmd/app/resolve_discussion_test.go @@ -78,7 +78,7 @@ func TestResolveDiscussion(t *testing.T) { request := makeRequest(t, http.MethodPut, "/mr/discussions/resolve", testResolveMergeRequestPayload) data, status := getFailData(t, svc, request) assert(t, data.Message, "Could not resolve discussion") - assert(t, data.Details, "Some error from Gitlab") + assert(t, data.Details, "some error from Gitlab") assert(t, status, http.StatusInternalServerError) }) } diff --git a/cmd/app/server.go b/cmd/app/server.go index 342eb947..a9f97497 100644 --- a/cmd/app/server.go +++ b/cmd/app/server.go @@ -86,7 +86,13 @@ func CreateRouter(gitlabClient *Client, projectInfo *ProjectInfo, s *shutdownSer for _, optFunc := range optFuncs { err := optFunc(&d) if err != nil { - panic(err) + if os.Getenv("DEBUG") != "" { + // TODO: We have some JSON files (emojis.json) we import relative to the binary in production and + // expect to break during debugging, do not throw when that occurs. + fmt.Fprintf(os.Stdout, "Issue occured setting up router: %s\n", err) + } else { + panic(err) + } } } @@ -245,13 +251,13 @@ func CreateRouter(gitlabClient *Client, projectInfo *ProjectInfo, s *shutdownSer func checkServer(port int) error { for i := 0; i < 10; i++ { resp, err := http.Get("http://localhost:" + fmt.Sprintf("%d", port) + "/ping") - if resp.StatusCode == 200 && err == nil { + if resp != nil && resp.StatusCode == 200 && err == nil { return nil } time.Sleep(100 * time.Microsecond) } - return errors.New("Could not start server!") + return errors.New("could not start server") } /* Creates a TCP listener on the port specified by the user or a random port */ diff --git a/cmd/app/test_helpers.go b/cmd/app/test_helpers.go index 98ec8d2b..2c3e1bf8 100644 --- a/cmd/app/test_helpers.go +++ b/cmd/app/test_helpers.go @@ -14,7 +14,7 @@ import ( "github.com/xanzy/go-gitlab" ) -var errorFromGitlab = errors.New("Some error from Gitlab") +var errorFromGitlab = errors.New("some error from Gitlab") /* The assert function is a helper function used to check two comparables */ func assert[T comparable](t *testing.T, got T, want T) { diff --git a/cmd/main.go b/cmd/main.go index 175aa30a..b46a880f 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -14,6 +14,10 @@ var pluginOptions app.PluginOptions func main() { log.SetFlags(0) + if len(os.Args) < 2 { + log.Fatal("Must provide server configuration") + } + err := json.Unmarshal([]byte(os.Args[1]), &pluginOptions) app.SetPluginOptions(pluginOptions) @@ -28,12 +32,12 @@ func main() { log.Fatalf("Failure initializing plugin: %v", err) } - err, client := app.NewClient() + client, err := app.NewClient() if err != nil { log.Fatalf("Failed to initialize Gitlab client: %v", err) } - err, projectInfo := app.InitProjectSettings(client, gitData) + projectInfo, err := app.InitProjectSettings(client, gitData) if err != nil { log.Fatalf("Failed to initialize project settings: %v", err) }