-
Notifications
You must be signed in to change notification settings - Fork 560
Add helpful targets via Makefile #211
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.
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
Conversation
Signed-off-by: Radoslav Dimitrov <[email protected]>
Signed-off-by: Radoslav Dimitrov <[email protected]>
…h curl Signed-off-by: Radoslav Dimitrov <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@domdomegg - hey, I realised you merged #144 so I rebased my changes and resolved the conflicts 👍 |
|
Thanks! Yep sorry, going through the backlog of PRs in chronological order and just got to this one :) |
It's alright, I figured something like that happened 🙏 I also felt bad for taking over the other person's change so this was for the better 😃 |
domdomegg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably fine to merge - would appreciate you looking over the comments, but will probably merge by default to unblock things in a bit if you're busy now and then we can fix more stuff on top.
| - name: Build application | ||
| run: | | ||
| go build -v ./cmd/... | ||
| run: make build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍
(in line with my 'prefer using make commands locally and in CI to keep them in sync')
| - return | ||
| nestif: | ||
| min-complexity: 8 | ||
| revive: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uncertain: Do we need this extra rule?
I'm not super familiar with golangci-lint, so apologies if this is a dumb question! Overall I think I'd like us to reduce custom config and use whatever the common/popular presets are: makes onboarding to the repo easier and AI tools more likely to get things right.
|
Maybe helpful for review: https://adamjones.me/blog/reviewing-your-work/ Although probably nothing there is too surprising |
## Motivation and Context <!-- Why is this change needed? What problem does it solve? --> The following PR: * Adds a Makefile with the most common project-related commands * Updates the CI workflow to leverage that (people testing locally will use the same commands as the CI) * Switches to using the official Golangci-lint action instead of downloading it via curl * Bumps the golangci version (supersedes #144) * Fixes any formatting/linting issues found * Updates the README to reference the new make targets * Runs the schema validation scripts as part of the CI **Next:** * If you are okay I can file a follow up PR afterwards where I can tidy up the CI workflows a bit, it feels there's some overlap. ## How Has This Been Tested? <!-- Have you tested this in a real application? Which scenarios were tested? --> Locally by running the make targets ## Breaking Changes <!-- Will users need to update their code or configurations? --> ## Types of changes <!-- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [ ] Bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation update ## Checklist <!-- Go over all the following points, and put an `x` in all the boxes that apply. --> - [x] I have read the [MCP Documentation](https://modelcontextprotocol.io) - [x] My code follows the repository's style guidelines - [x] New and existing tests pass locally - [ ] I have added appropriate error handling - [ ] I have added or updated documentation as needed ## Additional context <!-- Add any other context, implementation notes, or design decisions --> --------- Signed-off-by: Radoslav Dimitrov <[email protected]> Co-authored-by: Adam Jones <[email protected]>
Motivation and Context
The following PR:
Next:
How Has This Been Tested?
Locally by running the make targets
Breaking Changes
Types of changes
Checklist
Additional context