From f8e1f7bce21be0961da16333b53820ce93b2a542 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 19 Jun 2024 10:44:23 +0200 Subject: [PATCH 1/4] docs: Improve contributing docs --- CONTRIBUTING.md | 74 ++++-------------------------- docs/commit-issue-pr-guidelines.md | 39 ++++++++++++++++ docs/pr-reviews.md | 35 ++++++++++++++ 3 files changed, 84 insertions(+), 64 deletions(-) create mode 100644 docs/commit-issue-pr-guidelines.md create mode 100644 docs/pr-reviews.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 161cd60ef540..46f27e3ebed8 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -112,79 +112,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..3db9b2a47025 --- /dev/null +++ b/docs/commit-issue-pr-guidelines.md @@ -0,0 +1,39 @@ +# 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 `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..55401669344d --- /dev/null +++ b/docs/pr-reviews.md @@ -0,0 +1,35 @@ +# 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) From 1c3a9205961067f0359c2d867fc18c90f7cc29d7 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 19 Jun 2024 11:28:19 +0200 Subject: [PATCH 2/4] small adaptions --- CONTRIBUTING.md | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 46f27e3ebed8..b830e99ed6a1 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -58,11 +58,7 @@ 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 From afdb9aff2be6186b218402eea1cb0041508e42b4 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 19 Jun 2024 11:37:26 +0200 Subject: [PATCH 3/4] fix prettier --- CONTRIBUTING.md | 8 +++++--- docs/commit-issue-pr-guidelines.md | 3 ++- docs/pr-reviews.md | 19 +++++++++++++------ 3 files changed, 20 insertions(+), 10 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b830e99ed6a1..215527c16495 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -58,7 +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. -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`. +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 @@ -110,8 +112,8 @@ Note: you must run `yarn build` before `yarn lint` will work. ## External Contributors -We highly appreciate external contributions to the SDK. -If you want to contribute something, you can just open a PR against `develop`. +We highly appreciate external contributions to the SDK. If you want to contribute something, you can just open a PR +against `develop`. The SDK team will check out your PR shortly! diff --git a/docs/commit-issue-pr-guidelines.md b/docs/commit-issue-pr-guidelines.md index 3db9b2a47025..4bc5cd4195d0 100644 --- a/docs/commit-issue-pr-guidelines.md +++ b/docs/commit-issue-pr-guidelines.md @@ -30,7 +30,8 @@ and committed as such onto master. - 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. +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 diff --git a/docs/pr-reviews.md b/docs/pr-reviews.md index 55401669344d..87b5faafb950 100644 --- a/docs/pr-reviews.md +++ b/docs/pr-reviews.md @@ -10,7 +10,9 @@ important a comment is: - `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. +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: @@ -22,14 +24,19 @@ Our different types of reviews: `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. +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! +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. +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) From 9b61ec0da8f33d7dc3c099b95341e302641b6291 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 19 Jun 2024 12:54:42 +0200 Subject: [PATCH 4/4] Update docs/commit-issue-pr-guidelines.md Co-authored-by: Andrei <168741329+andreiborza@users.noreply.github.com> --- docs/commit-issue-pr-guidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/commit-issue-pr-guidelines.md b/docs/commit-issue-pr-guidelines.md index 4bc5cd4195d0..7c630dc907af 100644 --- a/docs/commit-issue-pr-guidelines.md +++ b/docs/commit-issue-pr-guidelines.md @@ -22,7 +22,7 @@ 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. +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`)