-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
docs: Improve contributing docs (#12541)
See: https://github.com/getsentry/sentry-javascript/blob/fn/contributing-docs/CONTRIBUTING.md --------- Co-authored-by: Andrei <168741329+andreiborza@users.noreply.github.com>
- Loading branch information
1 parent
0019309
commit 31700c9
Showing
3 changed files
with
95 additions
and
69 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| # Commit, Issue & PR guidelines | ||
|
|
||
| ## Commits | ||
|
|
||
| For commit messages, we use the format: | ||
|
|
||
| ``` | ||
| <type>(<scope>): <subject> (<github-id>) | ||
| ``` | ||
|
|
||
| For example: `feat(core): Set custom transaction source for event processors (#5722)`. | ||
|
|
||
| See [commit message format](https://develop.sentry.dev/commit-messages/#commit-message-format) for details. | ||
|
|
||
| The Github-ID can be left out until the PR is merged. | ||
|
|
||
| ## Issues | ||
|
|
||
| Issues should at least be categorized by package, for example `package: Node`. Additional labels for categorization can | ||
| be added, and the Sentry SDK team may also add further labels as needed. | ||
|
|
||
| ## Pull Requests (PRs) | ||
|
|
||
| PRs are merged via `Squash and merge`. This means that all commits on the branch will be squashed into a single commit, | ||
| and committed as such onto `develop`. | ||
|
|
||
| - The PR name can generally follow the commit name (e.g. | ||
| `feat(core): Set custom transaction source for event processors`) | ||
| - Make sure to rebase the branch on `develop` before squashing it | ||
| - Make sure to update the commit message of the squashed branch to follow the commit guidelines - including the PR | ||
| number | ||
|
|
||
| Please note that we cannot _enforce_ Squash Merge due to the usage of Gitflow (see below). Github remembers the last | ||
| used merge method, so you'll need to make sure to double check that you are using "Squash and Merge" correctly. | ||
|
|
||
| ## Gitflow | ||
|
|
||
| We use [Gitflow](https://docs.github.com/en/get-started/quickstart/github-flow) as a branching model. | ||
|
|
||
| For more details, [see our Gitflow docs](./gitflow.md). |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| # PR reviews | ||
|
|
||
| Make sure to open PRs against `develop` branch. | ||
|
|
||
| For feedback in PRs, we use the [LOGAF scale](https://blog.danlew.net/2020/04/15/the-logaf-scale/) to specify how | ||
| important a comment is: | ||
|
|
||
| - `l`: low - nitpick. You may address this comment, but you don't have to. | ||
| - `m`: medium - normal comment. Worth addressing and fixing. | ||
| - `h`: high - Very important. We must not merge this PR without addressing this issue. | ||
|
|
||
| You only need one approval from a maintainer to be able to merge. For some PRs, asking specific or multiple people for | ||
| review might be adequate. You can either assign SDK team members directly (e.g. if you have some people in mind who are | ||
| well suited to review a PR), or you can assign `getsentry/team-web-sdk-frontend`, which will randomly pick 2 people from | ||
| the team to assign. | ||
|
|
||
| Our different types of reviews: | ||
|
|
||
| 1. **LGTM without any comments.** You can merge immediately. | ||
| 2. **LGTM with low and medium comments.** The reviewer trusts you to resolve these comments yourself, and you don't need | ||
| to wait for another approval. | ||
| 3. **Only comments.** You must address all the comments and need another review until you merge. | ||
| 4. **Request changes.** Only use if something critical is in the PR that absolutely must be addressed. We usually use | ||
| `h` comments for that. When someone requests changes, the same person must approve the changes to allow merging. Use | ||
| this sparingly. | ||
|
|
||
| You show generally avoid to use "Auto merge". The reason is that we have some CI workflows which do not block merging | ||
| (e.g. flaky test detection, some optional E2E tests). If these fail, and you enabled Auto Merge, the PR will be merged | ||
| if though some workflow(s) failed. To avoid this, wait for CI to pass to merge the PR manually, or only enable "Auto | ||
| Merge" if you know that no optional workflow may fail. Another reason is that, as stated above in 2., reviewers may | ||
| leave comments and directly approve the PR. In this case, as PR author you should review the comments and choose which | ||
| to implement and which may be ignored for now. "Auto Merge" leads to the PR feedback not being taken into account. | ||
|
|
||
| ## Reviewing a PR from an external contributor | ||
|
|
||
| 1. Make sure to review PRs from external contributors in a timely fashion. These users spent their valuable time to | ||
| improve our SDK, so we should not leave them hanging with a review! | ||
| 2. Make sure to click "Approve and Run" on the CI for the PR, if it does not seem malicious. | ||
| 3. Provide feedback and guidance if the PR is not ready to be merged. | ||
| 4. Assign the PR to yourself if you start reviewing it. You are then responsible for guiding the PR either to | ||
| completion, or to close it if it does not align with the goals of the SDK team. | ||
| 5. Make sure to update the PR name to align with our commit name structure (see above) |