Add code quality tooling (Makefile, golangci-lint)#26
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces foundational code quality and developer experience improvements by integrating Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces code quality tooling with a Makefile and golangci-lint configuration, which is a great step for the project. My review focuses on modernizing the linter configuration and slightly improving the Makefile for clarity. I've suggested using the current, non-deprecated keys in .golangci.yml and simplifying a build command in the Makefile. These changes will improve the maintainability of the new tooling.
| timeout: 5m | ||
| skip-dirs: | ||
| - vendor |
There was a problem hiding this comment.
The run.skip-dirs option is deprecated in golangci-lint. It's recommended to handle all exclusions under the issues section for consistency. Since issues.exclude-dirs is also used, this is redundant.
timeout: 5m| exclude-dirs: | ||
| - vendor |
There was a problem hiding this comment.
The issues.exclude-dirs option is deprecated. Please use issues.exclude-rules with a path filter instead. This is the modern way to configure exclusions in golangci-lint.
exclude-rules:
- path: vendor/| # Build orchestrator binary | ||
| build: | ||
| @mkdir -p bin | ||
| go build -o bin/orchestrator ./go/cmd/orchestrator/main.go |
There was a problem hiding this comment.
You can simplify the go build command by providing the package path instead of the path to the main.go file. go build will automatically find the main package within that directory.
go build -o bin/orchestrator ./go/cmd/orchestrator
There was a problem hiding this comment.
Pull request overview
Adds contributor-facing code quality tooling at the repo root to standardize common dev workflows (build/test/format/lint) and to introduce a baseline golangci-lint configuration consistent with existing Go formatting guidance in the repo.
Changes:
- Add a root
Makefilewith common targets (build,test,lint,fmt,check-fmt,clean). - Add
.golangci.ymlenabling a conservative set of linters with a 5-minute timeout and vendor exclusion.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| Makefile | Provides standardized local commands for building, testing, linting, and formatting. |
| .golangci.yml | Configures golangci-lint with a limited linter set and vendor exclusions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Build orchestrator binary | ||
| build: | ||
| @mkdir -p bin | ||
| go build -o bin/orchestrator ./go/cmd/orchestrator/main.go | ||
|
|
||
| # Run all unit tests | ||
| test: | ||
| go test ./go/... |
There was a problem hiding this comment.
PR description says this change "Closes #19", but the linked issue’s acceptance criteria includes documenting the new Makefile targets in CONTRIBUTING.md. CONTRIBUTING.md currently only documents ./script/build and go test commands, with no mention of make build/test/lint/fmt. Either add a short Makefile section to CONTRIBUTING.md (preferred) or avoid closing the issue in this PR.
| run: | ||
| timeout: 5m | ||
| skip-dirs: | ||
| - vendor | ||
|
|
There was a problem hiding this comment.
The vendor directory is excluded twice: via run.skip-dirs and also under issues.exclude-dirs. This is redundant and can make it harder to reason about what is being skipped vs just excluded from reporting. Consider keeping only one mechanism (typically run-level skipping for vendor) or add a brief comment explaining why both are needed.
|
|
||
| issues: | ||
| exclude-dirs: | ||
| - vendor |
There was a problem hiding this comment.
This issues.exclude-dirs: vendor duplicates the run.skip-dirs: vendor setting above. Consider removing one of them to avoid configuration drift and reduce confusion about whether vendor is skipped entirely or just excluded from issue reporting.
| issues: | |
| exclude-dirs: | |
| - vendor |
Summary
Makefilewith targets:build,test,lint,fmt,check-fmt,clean.golangci.ymlconfiguration with conservative linter set:govet,staticcheck,errcheck,ineffassign,unused,gosimpleContributors can now use
make build,make test,make lint, andmake fmtinstead of remembering individual commands.No lint fixes are included — this PR only sets up the tooling. Lint fixes will follow in separate PRs.
Closes #19
Test plan
make buildcompilesbin/orchestratorsuccessfullymake testruns the test suitemake fmtformats Go source filesmake check-fmtexits cleanly when code is formattedmake cleanremoves build artifacts