From b04b10f52a753519cdb0eaa56b431204ade3c18b Mon Sep 17 00:00:00 2001 From: bretg Date: Wed, 20 Nov 2024 14:07:31 -0500 Subject: [PATCH 1/3] Update code-reviews.md --- docs/developers/code-reviews.md | 29 +++++++++-------------------- 1 file changed, 9 insertions(+), 20 deletions(-) diff --git a/docs/developers/code-reviews.md b/docs/developers/code-reviews.md index cc8ed667849..82376d150ed 100644 --- a/docs/developers/code-reviews.md +++ b/docs/developers/code-reviews.md @@ -3,31 +3,17 @@ ## 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. +to a reviewer for review within 5 business days of being opened. +That person should either approve the changes or request changes within 5 business days of being assigned. If they're too busy, they should assign it to someone else who can review it within that timeframe. -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. - -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 @@ -43,4 +29,7 @@ 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: + - 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. From 9733347aecc87dc13df349c4d1ec97cff10efdd4 Mon Sep 17 00:00:00 2001 From: bretg Date: Wed, 20 Nov 2024 14:10:19 -0500 Subject: [PATCH 2/3] Update code-reviews.md --- docs/developers/code-reviews.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/developers/code-reviews.md b/docs/developers/code-reviews.md index 82376d150ed..36d96868646 100644 --- a/docs/developers/code-reviews.md +++ b/docs/developers/code-reviews.md @@ -31,5 +31,8 @@ explaining it. Are there better ways to achieve those goals? - 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. - 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* From 57b874efab3e5a83327cfb632747d9da56a44c96 Mon Sep 17 00:00:00 2001 From: bretg Date: Fri, 22 Nov 2024 13:16:35 -0500 Subject: [PATCH 3/3] Update code-reviews.md --- docs/developers/code-reviews.md | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/docs/developers/code-reviews.md b/docs/developers/code-reviews.md index 36d96868646..ba7fb0ee526 100644 --- a/docs/developers/code-reviews.md +++ b/docs/developers/code-reviews.md @@ -8,14 +8,16 @@ Anyone is free to review and comment on any [open pull requests](https://github. ## Process -New pull requests should be [assigned](https://help.github.com/articles/assigning-issues-and-pull-requests-to-other-github-users/) -to a reviewer for review within 5 business days of being opened. -That person should either approve the changes or request changes within 5 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 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. 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.