Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update code-reviews.md #3560

Merged
merged 3 commits into from
Nov 26, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 16 additions & 22 deletions docs/developers/code-reviews.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,33 +3,21 @@
## Standards
Anyone is free to review and comment on any [open pull requests](https://github.com/prebid/prebid-server-java/pulls).

All pull requests must be reviewed and approved by at least one [core member](https://github.com/orgs/prebid/teams/core/members) before merge.

Very small pull requests may be merged with just one review if they:

1. Do not change the public API.
2. Have low risk of bugs, in the opinion of the reviewer.
3. Introduce no new features, or impact the code architecture.

Larger pull requests must meet at least one of the following two additional requirements.

1. Have a second approval from a core member
2. Be open for 5 business days with no new changes requested.
1. PRs that touch only adapters and modules can be approved by one reviewer before merge.
2. PRs that touch PBS-core must be reviewed and approved by at least two 'core' reviewers before merge.

## Process

New pull requests should be [assigned](https://help.github.com/articles/assigning-issues-and-pull-requests-to-other-github-users/)
to a core member for review within 3 business days of being opened.
That person should either approve the changes or request changes within 4 business days of being assigned.
If they're too busy, they should assign it to someone else who can review it within that timeframe.
New pull requests must be [assigned](https://help.github.com/articles/assigning-issues-and-pull-requests-to-other-github-users/)
to a reviewer within 5 business days of being opened. That person must either approve the changes or request changes within 5 business days of being assigned.

If a reviewer is too busy, they should re-assign it to someone else as soon as possible so that person has enough time to take over the review and still meet the 5-day goal. Please tag the new reviewer in the PR. If you don't know who to assign it to, use the #prebid-server-java-dev Slack channel to ask for help in re-assigning.

If the changes are small, that member can merge the PR once the changes are complete. Otherwise, they should
assign the pull request to another member for a second review.
If a reviewer is going to be unavailable for more than a few days, they should update the notes column of the duty spreadsheet or drop a note about their availability into the Slack channel.

The pull request can then be merged whenever the second reviewer approves, or if 5 business days pass with no farther
changes requested by anybody, whichever comes first.
After the review, if the PR touches PBS-core, it must be assigned to a second reviewer.

## Priorities
## Review Priorities

Code reviews should focus on things which cannot be validated by machines.

Expand All @@ -43,4 +31,10 @@ explaining it. Are there better ways to achieve those goals?
- Does the code use any global, mutable state? [Inject dependencies](https://en.wikipedia.org/wiki/Dependency_injection) instead!
- Can the code be organized into smaller, more modular pieces?
- Is there dead code which can be deleted? Or TODO comments which should be resolved?
- Look for code used by other adapters. Encourage adapter submitter to utilize common code.
- Look for code used by other adapters. Encourage adapter submitter to utilize common code.
- Specific bid adapter rules:
- The email contact must work and be a group, not an individual.
- Host endpoints cannot be fully dynamic. i.e. they can utilize "https://REGION.example.com", but not "https://HOST".
- They cannot _require_ a "region" parameter. Region may be an optional parameter, but must have a default.
- No direct use of HTTP is prohibited - *implement an existing Bidder interface that will do all the job*
- If the ORTB is just forwarded to the endpoint, use the generic adapter - *define the new adapter as the alias of the generic adapter*
Loading