Skip to content

Commit 31700c9

Browse files
mydeaandreiborza
andauthored
1 parent 0019309 commit 31700c9

File tree

3 files changed

+95
-69
lines changed

3 files changed

+95
-69
lines changed

CONTRIBUTING.md

+13-69
Original file line numberDiff line numberDiff line change
@@ -58,11 +58,9 @@ To test local versions of SDK packages, for instance in test projects, you have
5858

5959
**Any nontrivial fixes/features should include tests.** You'll find a `test` folder in each package.
6060

61-
Note that _for the `browser` package only_, if you add a new file to the
62-
[integration test suite](https://github.com/getsentry/sentry-javascript/tree/master/packages/browser/test/integration/suites),
63-
you also need to add it to
64-
[the list in `shell.js`](https://github.com/getsentry/sentry-javascript/blob/b74e199254147fd984e7bb1ea24193aee70afa74/packages/browser/test/integration/suites/shell.js#L25)
65-
as well. Adding tests to existing files will work out of the box in all packages.
61+
For browser related changes, you may also add tests in `dev-packages/browser-integration-tests`. Similarly, for node
62+
integration tests can be added in `dev-packages/node-integration-tests`. Finally, we also have E2E test apps in
63+
`dev-packages/e2e-tests`.
6664

6765
## Running Tests
6866

@@ -112,79 +110,25 @@ Similar to building and testing, linting can be done in the project root or in i
112110

113111
Note: you must run `yarn build` before `yarn lint` will work.
114112

115-
## Considerations Before Sending Your First PR
113+
## External Contributors
116114

117-
When contributing to the codebase, please note:
118-
119-
- Make sure to follow the [Commit, Issue & PR guidelines](#commit-issue--pr-guidelines)
120-
- Non-trivial PRs will not be accepted without tests (see above).
121-
- Please do not bump version numbers yourself.
122-
- [`raven-js`](https://github.com/getsentry/sentry-javascript/tree/3.x/packages/raven-js) and
123-
[`raven-node`](https://github.com/getsentry/sentry-javascript/tree/3.x/packages/raven-node) are deprecated, and only
124-
bug and security fix PRs will be accepted targeting the
125-
[3.x branch](https://github.com/getsentry/sentry-javascript/tree/3.x). Any new features and improvements should be to
126-
our new SDKs (`browser`, `node`, and framework-specific packages like `react` and `nextjs`) and the packages which
127-
support them (`core`, `utils`, `integrations`, and the like).
128-
129-
## PR reviews
130-
131-
For feedback in PRs, we use the [LOGAF scale](https://blog.danlew.net/2020/04/15/the-logaf-scale/) to specify how
132-
important a comment is:
133-
134-
- `l`: low - nitpick. You may address this comment, but you don't have to.
135-
- `m`: medium - normal comment. Worth addressing and fixing.
136-
- `h`: high - Very important. We must not merge this PR without addressing this issue.
115+
We highly appreciate external contributions to the SDK. If you want to contribute something, you can just open a PR
116+
against `develop`.
137117

138-
You only need one approval from a maintainer to be able to merge. For some PRs, asking specific or multiple people for
139-
review might be adequate.
118+
The SDK team will check out your PR shortly!
140119

141-
Our different types of reviews:
120+
When contributing to the codebase, please note:
142121

143-
1. **LGTM without any comments.** You can merge immediately.
144-
2. **LGTM with low and medium comments.** The reviewer trusts you to resolve these comments yourself, and you don't need
145-
to wait for another approval.
146-
3. **Only comments.** You must address all the comments and need another review until you merge.
147-
4. **Request changes.** Only use if something critical is in the PR that absolutely must be addressed. We usually use
148-
`h` comments for that. When someone requests changes, the same person must approve the changes to allow merging. Use
149-
this sparingly.
122+
- Make sure to follow the [Commit, Issue & PR guidelines](./docs/commit-issue-pr-guidelines.md)
123+
- Non-trivial PRs will not be accepted without tests (see above).
150124

151125
## Commit, Issue & PR guidelines
152126

153-
### Commits
154-
155-
For commit messages, we use the format:
156-
157-
```
158-
<type>(<scope>): <subject> (<github-id>)
159-
```
160-
161-
For example: `feat(core): Set custom transaction source for event processors (#5722)`.
162-
163-
See [commit message format](https://develop.sentry.dev/commit-messages/#commit-message-format) for details.
164-
165-
The Github-ID can be left out until the PR is merged.
166-
167-
### Issues
168-
169-
Issues should at least be categorized by package, for example `package: Node`. Additional labels for categorization can
170-
be added, and the Sentry SDK team may also add further labels as needed.
171-
172-
### Pull Requests (PRs)
173-
174-
PRs are merged via `Squash and merge`. This means that all commits on the branch will be squashed into a single commit,
175-
and committed as such onto master.
176-
177-
- The PR name can generally follow the commit name (e.g.
178-
`feat(core): Set custom transaction source for event processors`)
179-
- Make sure to rebase the branch on `master` before squashing it
180-
- Make sure to update the commit message of the squashed branch to follow the commit guidelines - including the PR
181-
number
182-
183-
### Gitflow
127+
See [Commit, Issue & PR guidelines](./docs/commit-issue-pr-guidelines.md).
184128

185-
We use [Gitflow](https://docs.github.com/en/get-started/quickstart/github-flow) as a branching model.
129+
## PR Reviews
186130

187-
For more details, [see our Gitflow docs](./docs/gitflow.md).
131+
See [PR Reviews](./docs/pr-reviews.md).
188132

189133
## Publishing a Release
190134

docs/commit-issue-pr-guidelines.md

+40
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
# Commit, Issue & PR guidelines
2+
3+
## Commits
4+
5+
For commit messages, we use the format:
6+
7+
```
8+
<type>(<scope>): <subject> (<github-id>)
9+
```
10+
11+
For example: `feat(core): Set custom transaction source for event processors (#5722)`.
12+
13+
See [commit message format](https://develop.sentry.dev/commit-messages/#commit-message-format) for details.
14+
15+
The Github-ID can be left out until the PR is merged.
16+
17+
## Issues
18+
19+
Issues should at least be categorized by package, for example `package: Node`. Additional labels for categorization can
20+
be added, and the Sentry SDK team may also add further labels as needed.
21+
22+
## Pull Requests (PRs)
23+
24+
PRs are merged via `Squash and merge`. This means that all commits on the branch will be squashed into a single commit,
25+
and committed as such onto `develop`.
26+
27+
- The PR name can generally follow the commit name (e.g.
28+
`feat(core): Set custom transaction source for event processors`)
29+
- Make sure to rebase the branch on `develop` before squashing it
30+
- Make sure to update the commit message of the squashed branch to follow the commit guidelines - including the PR
31+
number
32+
33+
Please note that we cannot _enforce_ Squash Merge due to the usage of Gitflow (see below). Github remembers the last
34+
used merge method, so you'll need to make sure to double check that you are using "Squash and Merge" correctly.
35+
36+
## Gitflow
37+
38+
We use [Gitflow](https://docs.github.com/en/get-started/quickstart/github-flow) as a branching model.
39+
40+
For more details, [see our Gitflow docs](./gitflow.md).

docs/pr-reviews.md

+42
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
# PR reviews
2+
3+
Make sure to open PRs against `develop` branch.
4+
5+
For feedback in PRs, we use the [LOGAF scale](https://blog.danlew.net/2020/04/15/the-logaf-scale/) to specify how
6+
important a comment is:
7+
8+
- `l`: low - nitpick. You may address this comment, but you don't have to.
9+
- `m`: medium - normal comment. Worth addressing and fixing.
10+
- `h`: high - Very important. We must not merge this PR without addressing this issue.
11+
12+
You only need one approval from a maintainer to be able to merge. For some PRs, asking specific or multiple people for
13+
review might be adequate. You can either assign SDK team members directly (e.g. if you have some people in mind who are
14+
well suited to review a PR), or you can assign `getsentry/team-web-sdk-frontend`, which will randomly pick 2 people from
15+
the team to assign.
16+
17+
Our different types of reviews:
18+
19+
1. **LGTM without any comments.** You can merge immediately.
20+
2. **LGTM with low and medium comments.** The reviewer trusts you to resolve these comments yourself, and you don't need
21+
to wait for another approval.
22+
3. **Only comments.** You must address all the comments and need another review until you merge.
23+
4. **Request changes.** Only use if something critical is in the PR that absolutely must be addressed. We usually use
24+
`h` comments for that. When someone requests changes, the same person must approve the changes to allow merging. Use
25+
this sparingly.
26+
27+
You show generally avoid to use "Auto merge". The reason is that we have some CI workflows which do not block merging
28+
(e.g. flaky test detection, some optional E2E tests). If these fail, and you enabled Auto Merge, the PR will be merged
29+
if though some workflow(s) failed. To avoid this, wait for CI to pass to merge the PR manually, or only enable "Auto
30+
Merge" if you know that no optional workflow may fail. Another reason is that, as stated above in 2., reviewers may
31+
leave comments and directly approve the PR. In this case, as PR author you should review the comments and choose which
32+
to implement and which may be ignored for now. "Auto Merge" leads to the PR feedback not being taken into account.
33+
34+
## Reviewing a PR from an external contributor
35+
36+
1. Make sure to review PRs from external contributors in a timely fashion. These users spent their valuable time to
37+
improve our SDK, so we should not leave them hanging with a review!
38+
2. Make sure to click "Approve and Run" on the CI for the PR, if it does not seem malicious.
39+
3. Provide feedback and guidance if the PR is not ready to be merged.
40+
4. Assign the PR to yourself if you start reviewing it. You are then responsible for guiding the PR either to
41+
completion, or to close it if it does not align with the goals of the SDK team.
42+
5. Make sure to update the PR name to align with our commit name structure (see above)

0 commit comments

Comments
 (0)