From 4e307a4f9eb9ff550193bb4157d7324635905826 Mon Sep 17 00:00:00 2001 From: Matt Way Date: Wed, 18 Nov 2020 16:50:28 -0500 Subject: [PATCH] Update development guide (#2832) --- .github/pull_request_template.md | 6 +- DEVELOPER.md | 148 -------------------- DEVELOPMENT.md | 230 +++++++++++++++++++++++++++++++ README.md | 2 +- 4 files changed, 234 insertions(+), 152 deletions(-) delete mode 100644 DEVELOPER.md create mode 100644 DEVELOPMENT.md diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index f6c01cafe0..5e61c310a2 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -1,8 +1,8 @@ **What this PR does / why we need it**: diff --git a/DEVELOPER.md b/DEVELOPER.md deleted file mode 100644 index c0704b4e25..0000000000 --- a/DEVELOPER.md +++ /dev/null @@ -1,148 +0,0 @@ -# Developer Notes - -## Setup your development environment - -First fork https://github.com/m3db/m3 into your own workspace on Github. - -Then create your clone: - -```bash -export working_dir=$GOPATH/src/github.com/m3db -mkdir -p $working_dir - -# Set this to your Github user -export user="your github profile name" - -# Clone your fork -cd $working_dir -git clone git@github.com:$user/m3.git -# or: https://github.com/$user/m3.git - -# Set upstream -cd m3 -git remote add upstream git@github.com:m3db/m3.git -# or: https://github.com/m3db/m3.git - -# Never push to upstream master -git remote set-url --push upstream no_push - -# Check if it makes sense: -git remote -v -``` - -Install dependencies: - -```bash -cd $working_dir/m3 - -make install-vendor-m3 -``` - -If everything is setup correctly you should be able to build `m3dbnode`: - -```bash -make m3dbnode -``` - -## Running the M3 stack locally - -Follow the instructions in [this README](./scripts/development/m3_stack/README.md). - -## Testing Changes - -M3 has an extensive, and ever increasing, set of tests to ensure we are able to validate changes. More notes about the various testing strategies we employ can be found in `TESTING.md`. An unfortunate consequence of the number of tests is running the test suite takes too long on a developer's laptop. Here's the workflow most developers employ to be productive. Note: take this as a suggestion of something that works for some people, not as a directive. Do what makes you enjoy the development process most, including disregarding this suggestion! - -Once you have identified a change you want to make, and gathered consensus by talking to some devs, go ahead and make a branch with the changes. To test your changes: - -(0) If you have updated an interface that has been mocked, you need to update the generated `gomock `files. - -```shell -# Generate mocks for all top level packages -make mock-gen - -# If you just want to generate it for a single package, -# replace xyz with the package you want to generate files for, e.g. dbnode -make mock-gen-xyz -``` - -(1) Run unit tests locally -``` -go test ./... -v -``` - -This usually runs within 1minute on most laptops. And catches basic compilation errors quickly. - -(2) Submit to CI, and continue to develop further. - -(3) Once the CI job finishes - investigate failures (if any), and reproduce locally. - -For e.g. if a Unit Tests `TestXYZ` in package `github.com/m3db/m3/xyz` failed, run the following: - -``` -$ go test ./xyz -run TestXYZ -race -v -``` - -Similarly, for 'Big Unit Tests'/'Integration' tests, you may have to provide an additional build tag argument, e.g. - -``` -# example integration test -$ go test ./integration -tags integration -run TestIndexBlockRotation -v - -# example big unit test -$ go test -tags big ./services/m3dbnode/main -run TestIndexEnabledServer -v -``` - -(4) Fix, rinse, and repeat. - -(5) Polish up the PR, ensure CI signs off, and coverage increases. Then ping someone to take a look and get feedback! - -## Adding a Changelog - -When you open a PR, if you have made are making a significant change you must also update the contents of `CHANGELOG.md` with a description of the changes and the PR number. You will not know the PR number ahead of time so this must be added as a subsequent commit to an open PR. - -The format of the change should be: -``` -- TYPE **COMPONENT:** Description (#PR_NUMBER) -``` - -The `TYPE` should be ommitted if it is a feature. If it is not a feature it should be one of: `FIX`, `PERF`. New types of changes should be added to this document before used if they cannot be categorized by the existing types. - -Here is an example of an additiong to the pending changelog: - -``` -# 0.4.5 (unreleased) - -- FIX **DB** Index data race in FST Segment reads (#938) -``` - -## Building the Docs - -The `docs` folder contains our documentation in Markdown files. These Markdown files are built into a static site using -[`mkdocs`](https://www.mkdocs.org/) with the [`mkdocs-material`](https://squidfunk.github.io/mkdocs-material/) theme. -Building the docs using our predefined `make` targets requires a working Docker installation: - -``` -# generate the docs in the `site/` directory -make docs-build - -# build docs and serve on localhost:8000 (with live reload) -make docs-serve - -# build the docs and auto-push to the `gh-pages` branch -make docs-deploy -``` - -## M3 Website - -The [M3 website](https://m3db.io/) is hosted with Netlify. It is configured to run `make site-build` and then serving the contents of the `/m3db.io` directory. The site is built and republished every time -there is a push to master. - -## Cutting a release - -1. Ensure you have a Github API access token with the `repo` scope -2. Checkout the commit you want to release (do not release commits that don't exist on master) -3. Create a tag with the version you would like to release. Ex: `git tag -a v0.7.0 -m "v0.7.0"` -4. Push the tag. Ex: `git push origin v0.7.0` -5. Run `make GITHUB_TOKEN= release` -6. Update `CHANGELOG.md` and commit it to master -7. Copy and paste the text from `CHANGELOG.md` into the release notes [on Github](https://github.com/m3db/m3/releases) diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md new file mode 100644 index 0000000000..2b1ec53188 --- /dev/null +++ b/DEVELOPMENT.md @@ -0,0 +1,230 @@ +# Development + +M3 is a large, active codebase. This guide is intended for all potential +contributors, designed to make it easy to get started with contributing to M3. + +## Setup your development environment + +First, fork https://github.com/m3db/m3 to your own user or org account on GitHub. + +Then, clone your fork: + +```bash +mkdir -p $GOPATH/src/github.com/m3db +cd $GOPATH/src/github.com/m3db +git clone git@github.com:/m3.git +``` + +Note: All commits/branches/etc must be pushed to the fork, not to m3db/m3. + +Verify that `git clone` properly set the repository's upstream: + +``` +# git remote -v +origin git@github.com:/m3.git (fetch) +origin git@github.com:/m3.git (push) +``` + +Install dependencies: + +```bash +cd m3/ +make install-vendor-m3 +``` + +If everything is set up correctly, `m3dbnode` should build successfully: + +```bash +make m3dbnode +``` + +## Running the M3 stack locally + +Follow the instructions in [this README][local-readme]. + +[local-readme]: ./scripts/development/m3_stack/README.md + +## Updating Mocks And Generated Files + +If changes require updates to mocks or other generated files, make sure to +update those files. There are `make` targets to help with generation: + +- Mocks: `make mock-gen` +- Protobuf: `make proto-gen` +- Thrift: `make thrift-gen` + +Don't forget to account for changes to generated files in tests! + +## Testing Changes + +M3 has an extensive test suite to ensure that we are able to validate changes. +More notes about the various testing strategies we employ can be found in +[TESTING][TESTING.md]. + +While our CI suite will run all tests prior to allowing pull requests to be +merged, developers should test their changes during development and prior to +pushing updates. To test code, use `go test`: + +``` +go test -v ./src/... +``` + +Unfortunately, as more tests have been added, the time required to run the +entire suite has increased significantly; therefore, we recommend at least +running tests for both (1) any packages with changes and (2) any packages that +import the changed packages. An example of this would be: + +```bash +changed=$(git diff --name-only HEAD^ HEAD | xargs -I {} dirname {} | sort | uniq) +for pkg in $changed; do + affected=$(grep -r "$pkg" ./src | cut -d: -f1 | grep -v mock | xargs -I{} dirname {} | sort | uniq) + go test -v -race "./$pkg" $affected +done +``` + +Contributors are free to do whatever due diligence that they feel helps them to +be most productive. Our only request is that CI jobs are not used as a +first-pass filter to determine whether a change is sane or not. + +Once tests are passing locally, push to a new remote branch to create a pull +request, or push to an existing branch to update a pull request. If the CI +suite reports any errors, attempt to reproduce failures locally and fix them +before continuing. + +For larger or more intensive tests (e.g. "big" unit tests, integration tests), +additional build tags may be necessary to scope the tests down to a smaller +subset, e.g.: + +``` +# example integration test +$ go test ./integration -tags integration -run TestIndexBlockRotation -v + +# example big unit test +$ go test -tags big ./services/m3dbnode/main -run TestIndexEnabledServer -v +``` + +## Code Review + +Please observe the following guidelines when submitting or reviewing pull +requests: + +- Merging is blocked on approval by 1 or more users with write access. We + recommend 2+ for large, complex, or nuanced changes. +- Pull requests should contain clear descriptions and context per the + pull request template. +- Breaking changes under the current versioning guarantees (see + [COMPATIBILITY][COMPATIBILITY.md]) must be approved by the + [Technical Steering Committee (TSC)][GOVERNANCE.md]. +- The [STYLEGUIDE][STYLEGUIDE.md] should be followed to the extent reasonable + within the scope of the pull request. +- Changed codepaths should be validated by unit tests that cover at least one + nominal case and one error case. +- Pull requests may only be merged with a green build. Do not merge with + build failures, even if they are known and unrelated. +- Flaky or otherwise unreliable CI failures count as hard failures. Commit fixes + or skips for flaky tests or other failures first! + +### Scoping pull requests + +Significantly inspired by Phabricator's article about +[Recommendations on Revision Control][phab-one-idea], and particularly +because pull requests tend to be squash-merged, try to keep PRs focused +on one idea (or the minimal number of ideas for the change to be viable). + +Some of the advantages of smaller PRs are: +- quicker to write + quicker to review = faster iteration +- easier to spot errors +- less cognitive overhead (and net time invested) for reviewers due to + reduced scope +- naturally avoids scope creep +- clearly establishes that all parts of the PR are related + +Because of this, contributors are encouraged to keep PRs as small as +possible. Given that this does introduce some developer overhead (e.g. +needing to manage more PRs), *how* small is unspecified; reviewers +can request PRs to be broken down further as necessary, and contributors +should work with reviewers to find the right balance. + +[phab-one-idea]: https://secure.phabricator.com/book/phabflavor/article/recommendations_on_revision_control/#one-idea-is-one-commit + +## Updating the CHANGELOG + +After a pull request is merged, significant changes must be summarized in the +[CHANGELOG][CHANGELOG.md]. Since the PR number is not known ahead of time, this +must be done as a subsquent commit after the merged PR (though the CHANGELOG +PR can be prepared in tandem with the referenced PR). + +The format of the CHANGELOG entry should be: + +``` +- TYPE **COMPONENT**: Description (#PR_NUMBER) +``` + +The `TYPE` should be ommitted if it is a feature. If the change is not a +feature, `TYPE` should be either `FIX` or `PERF`. New types of changes should +be added to this document before used if they cannot be categorized by the +existing types. + +New CHANGELOG entries should be added to the current "unreleased" section at +the top of the CHANGELOG as long as they are within the scope of that potential +version. If no unreleased section exists, one should be added using the +appropriate proposed semver. If an unreleased section exists but the new change +requires a different semver change (e.g. the unreleased version is a patch +version bump, but the new change requires a minor version bump), the version +on the existing section should be updated. See [COMPATIBILITY][COMPATIBILITY.md] +for more information on versioning. + +An example CHANGELOG addition: + +``` +# 0.4.5 (unreleased) + +- FIX **DB**: Index data race in FST Segment reads (#938) +``` + +## Building Docs + +Documentation is located adjacent to other [m3db.io][m3db.io] assets in +[site/content/docs/][docs-path]. These markdown files are built into a static +site using Hugo and Docker through `make docs-build`. + +Documentation changes can be tested locally by simply running a Hugo webserver +(installing Hugo is required for this step: simply `brew install hugo` or see +Hugo's [Quick Start][hugo-quick-start] for more options). Simply run: + +``` +cd site +hugo server +``` + +once Hugo is installed to run a local webserver that will live-update as +changes are made to the documentation or rest of the site. + +## M3 Website + +The M3 website, [m3db.io][m3db.io], is hosted with Netlify. It is configured to +run `make site-build` and then serve the contents of the `site/public/` +directory. The site is built and republished on every commit to master. + +## Cutting a release + +1. Ensure you have a GitHub API access token with the `repo` scope +2. Checkout the commit you want to release (all releases must be on master) +3. Create a tag with the version you would like to release + - E.g. `git tag -a v0.7.0 -m "v0.7.0"` + - See [COMPATIBILITY][COMPATIBILITY.md] for semver information. +4. Push the tag + - E.g. `git push origin v0.7.0` +5. Run `make GITHUB_TOKEN= release` +6. Update `CHANGELOG.md` and commit it to master +7. Copy and paste the text from `CHANGELOG.md` into the release notes [on GitHub][releases]. + +[CHANGELOG.md]: https://github.com/m3db/m3/blob/master/CHANGELOG.md +[COMPATIBILITY.md]: https://github.com/m3db/m3/blob/master/COMPATIBILITY.md +[GOVERNANCE.md]: https://github.com/m3db/m3/blob/master/GOVERNANCE.md +[STYLEGUIDE.md]: https://github.com/m3db/m3/blob/master/STYLEGUIDE.md +[TESTING.md]: https://github.com/m3db/m3/blob/master/TESTING.md +[docs-path]: https://github.com/m3db/m3/tree/master/site/content/docs +[hugo-quick-start]: https://gohugo.io/getting-started/quick-start/ +[m3db.io]: https://m3db.io/ +[releases]: https://github.com/m3db/m3/releases diff --git a/README.md b/README.md index 8976d608a2..58cff0ce37 100644 --- a/README.md +++ b/README.md @@ -7,7 +7,7 @@ More information: - [Documentation](https://docs.m3db.io/) -- [Developer: Getting Started](https://github.com/m3db/m3/blob/master/DEVELOPER.md) +- [Developer Guide: Getting Started](https://github.com/m3db/m3/blob/master/DEVELOPMENT.md) - [Slack](http://bit.ly/m3slack) - [Forum (Google Group)](https://groups.google.com/forum/#!forum/m3db) - [Twitter](https://twitter.com/m3metrics)