-
Notifications
You must be signed in to change notification settings - Fork 50
docs: dev contrib doc #776
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: Sertac Ozercan <sozercan@gmail.com>
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.
Pull Request Overview
This PR adds comprehensive developer documentation to help contributors set up their development environment and understand the development workflow. The documentation addresses issue #770 by providing detailed guidance for new developers.
Key changes:
- Comprehensive developer guide covering setup, workflows, and best practices
- Complete Makefile for standardized development tasks
- Integration of developer documentation into the website navigation
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| website/docs/developers.md | New comprehensive developer guide with setup instructions, workflows, and best practices |
| website/sidebars.ts | Adds "Contributing" section to navigation with link to developer guide |
| Makefile | New comprehensive Makefile with standardized development, testing, and build targets |
| CONTRIBUTING.md | Updates main contributing guide to reference the new developer documentation |
cpuguy83
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.
I think a doc that gets more in the nitty-gritty details of how to run unit tests vs integration tests, where these live, how the integration framework works, etc would be more beneficial here?
| @@ -0,0 +1,206 @@ | |||
| .PHONY: help | |||
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.
I am generally not a fan of Makefiles like this.
Its not adding anything over go generate ./... and go test.
We do have a docker-bake.hcl which can be useful for executing linters and certain tests and is generally very helpful for lazily building the frontend and feeding that into a build of a spec file without having to save the frontend to an image.
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.
i think mainly it adds discoverability. users don't really know that they need to run go generate until their PR fails the check
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.
Would this doc be how this stuff is discovered?
- Update required Go version to 1.24 - Remove unnecessary Makefile targets (build-redirectio, build-all-tools, test-short, test-verbose, version, mod-tidy, mod-verify) - Add TEST_FLAGS variable for flexibility instead of separate verbose target - Consolidate test-integration to handle both all tests and specific suites - Change docs-serve to use 'go -C ./website run .' instead of npm - Add comprehensive test structure documentation explaining unit vs integration tests - Document integration test framework and test file organization - Fix markdown linting issues Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
Makefile
Outdated
|
|
||
| .PHONY: generate | ||
| generate: ## Generate required source files | ||
| $(GO) generate ./.../cmd/lin |
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.
| $(GO) generate ./.../cmd/lin | |
| $(GO) generate ./... |
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when the PR gets merged):Fixes #770
Special notes for your reviewer: