diff --git a/pr_review.md b/PR_REVIEW.md similarity index 52% rename from pr_review.md rename to PR_REVIEW.md index 1d8cf0c360c..80d11c35769 100644 --- a/pr_review.md +++ b/PR_REVIEW.md @@ -1,5 +1,9 @@ ## Summary -We take PR review seriously. Please read https://medium.com/@mrjoelkemp/giving-better-code-reviews-16109e0fdd36#.xa8lc4i23 to understand how a PR review should be conducted. Be rational and strict in your review, make sure you understand exactly what the submitter's intent is. Overall 1 person should take ownership of a particular PR. When they are satisfied it's in good condition to merge, they should request 1 additional team member to review as a sanity check. Only when the PR has 2 `LGTM` from the core team should it be merged. +We take PR review seriously. Please read https://medium.com/@mrjoelkemp/giving-better-code-reviews-16109e0fdd36#.xa8lc4i23 to understand how a PR review should be conducted. Be rational and strict in your review, make sure you understand exactly what the submitter's intent is. Anyone in the community can review a PR, but a Prebid Org member is also required. A Prebid Org member should take ownership of a PR and do the initial review. + +If the PR is for a standard bid adapter or a standard analytics adapter, just the one review from a core member is sufficient. The reviewer will check against [required conventions](http://prebid.org/dev-docs/bidder-adaptor.html#required-adapter-conventions) and may merge the PR after approving and confirming that the documentation PR against prebid.org is open and linked to the issue. + +For modules and core platform updates, the initial reviewer should request an additional team member to review as a sanity check. Merge should only happen when the PR has 2 `LGTM` from the core team and a documentation PR if required. ### General PR review Process - Checkout the branch (these instructions are available on the github PR page as well). @@ -13,7 +17,7 @@ We take PR review seriously. Please read https://medium.com/@mrjoelkemp/giving-b - If all above is good, add a `LGTM` comment and request 1 additional core member to review. - Once there is 2 `LGTM` on the PR, merge to master - Ask the submitter to add a PR for documentation if applicable. -- Add a line into the `draft release` notes for this submission. If no draft release is available, create one using this template https://gist.github.com/mkendall07/c3af6f4691bed8a46738b3675cb5a479 +- Add a line into the `draft release` notes for this submission. If no draft release is available, create one using [this template]( https://gist.github.com/mkendall07/c3af6f4691bed8a46738b3675cb5a479) ### New Adapter or updates to adapter process - Follow steps above for general review process. In addition, please verify the following: @@ -23,3 +27,20 @@ We take PR review seriously. Please read https://medium.com/@mrjoelkemp/giving-b - Verify that code re-use is being done properly and that changes introduced by a bidder don't impact other bidders. - If the adapter being submitted is an alias type, check with the bidder contact that is being aliased to make sure it's allowed. - If the adapter is triggering any user syncs make sure they are using the user sync module in the Prebid.js core. +- Requests to the bidder should support HTTPS +- Responses from the bidder should be compressed (such as gzip, compress, deflate) +- Bid responses may not use JSONP: All requests must be AJAX with JSON responses +- All user-sync (aka pixel) activity must be registered via the provided functions +- Adapters may not use the $$PREBID_GLOBAL$$ variable +- All adapters must support the creation of multiple concurrent instances. This means, for example, that adapters cannot rely on mutable global variables. + +## Ticket Coordinator + +Each week, Prebid Org assigns one person to keep an eye on incoming issues and PRs. That person should: +- Review issues and PRs at least once per weekday for new items. +- For PRs: assign PRs to individuals on the PR review list. Try to be equitable -- not all PRs are created equally. Use the "Assigned" field and add the "Needs Review" label. +- For Issues: try to address questions and troubleshooting requests on your own, assigning them to others as needed. +- Issues that are questions or troubleshooting requests may be closed if the originator doesn't respond within a week to requests for confirmation or details. +- Issues that are bug reports should be left open and assigned to someone in PR rotation to confirm or deny the bug status. +- It's polite to check with others before assigning them large tasks. +- If possible, check in on older items and see if they can be unstuck.