diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 161cd60ef540..215527c16495 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -58,11 +58,9 @@ To test local versions of SDK packages, for instance in test projects, you have **Any nontrivial fixes/features should include tests.** You'll find a `test` folder in each package. -Note that _for the `browser` package only_, if you add a new file to the -[integration test suite](https://github.com/getsentry/sentry-javascript/tree/master/packages/browser/test/integration/suites), -you also need to add it to -[the list in `shell.js`](https://github.com/getsentry/sentry-javascript/blob/b74e199254147fd984e7bb1ea24193aee70afa74/packages/browser/test/integration/suites/shell.js#L25) -as well. Adding tests to existing files will work out of the box in all packages. +For browser related changes, you may also add tests in `dev-packages/browser-integration-tests`. Similarly, for node +integration tests can be added in `dev-packages/node-integration-tests`. Finally, we also have E2E test apps in +`dev-packages/e2e-tests`. ## Running Tests @@ -112,79 +110,25 @@ Similar to building and testing, linting can be done in the project root or in i Note: you must run `yarn build` before `yarn lint` will work. -## Considerations Before Sending Your First PR +## External Contributors -When contributing to the codebase, please note: - -- Make sure to follow the [Commit, Issue & PR guidelines](#commit-issue--pr-guidelines) -- Non-trivial PRs will not be accepted without tests (see above). -- Please do not bump version numbers yourself. -- [`raven-js`](https://github.com/getsentry/sentry-javascript/tree/3.x/packages/raven-js) and - [`raven-node`](https://github.com/getsentry/sentry-javascript/tree/3.x/packages/raven-node) are deprecated, and only - bug and security fix PRs will be accepted targeting the - [3.x branch](https://github.com/getsentry/sentry-javascript/tree/3.x). Any new features and improvements should be to - our new SDKs (`browser`, `node`, and framework-specific packages like `react` and `nextjs`) and the packages which - support them (`core`, `utils`, `integrations`, and the like). - -## PR reviews - -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. +We highly appreciate external contributions to the SDK. If you want to contribute something, you can just open a PR +against `develop`. -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. +The SDK team will check out your PR shortly! -Our different types of reviews: +When contributing to the codebase, please note: -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. +- Make sure to follow the [Commit, Issue & PR guidelines](./docs/commit-issue-pr-guidelines.md) +- Non-trivial PRs will not be accepted without tests (see above). ## Commit, Issue & PR guidelines -### Commits - -For commit messages, we use the format: - -``` -(): () -``` - -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 master. - -- 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 `master` before squashing it -- Make sure to update the commit message of the squashed branch to follow the commit guidelines - including the PR - number - -### Gitflow +See [Commit, Issue & PR guidelines](./docs/commit-issue-pr-guidelines.md). -We use [Gitflow](https://docs.github.com/en/get-started/quickstart/github-flow) as a branching model. +## PR Reviews -For more details, [see our Gitflow docs](./docs/gitflow.md). +See [PR Reviews](./docs/pr-reviews.md). ## Publishing a Release diff --git a/docs/commit-issue-pr-guidelines.md b/docs/commit-issue-pr-guidelines.md new file mode 100644 index 000000000000..7c630dc907af --- /dev/null +++ b/docs/commit-issue-pr-guidelines.md @@ -0,0 +1,40 @@ +# Commit, Issue & PR guidelines + +## Commits + +For commit messages, we use the format: + +``` +(): () +``` + +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). diff --git a/docs/pr-reviews.md b/docs/pr-reviews.md new file mode 100644 index 000000000000..87b5faafb950 --- /dev/null +++ b/docs/pr-reviews.md @@ -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)