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.

Sign up for GitHub

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

PR / Issue Review process update #2093

Merged
merged 3 commits into from
Feb 6, 2018
Merged

PR / Issue Review process update #2093

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a03111c
PR / Issue Review process update
bretg Feb 1, 2018
4cdb45b
Update pr_review.md
bretg Feb 1, 2018
e66dec0
Capitalize filename
matthewlane Feb 6, 2018
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
25 changes: 23 additions & 2 deletions pr_review.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just the one review [from a core team member] is sufficient


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).
Expand All @@ -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:
Expand All @@ -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 be in HTTPS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should support HTTPS (doesn't have to be HTTPS 100% of the time)

- Responses from the bidder should be compressed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.