Skip to content

Commit

Permalink
added pr review process and governance model (prebid#1103)
Browse files Browse the repository at this point in the history
* added pr review process and governance model

* fixed typo
  • Loading branch information
Matt Kendall authored Apr 10, 2017
1 parent 2ace03f commit a9ebedf
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 5 deletions.
15 changes: 10 additions & 5 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ This runs code quality checks, generates all the necessary files and starts a we

To run the example file, go to:

+ `http://localhost:9999/integrationExamples/gpt/pbjs_example_gpt.html`
+ `http://localhost:9999/integrationExamples/gpt/pbjs_example_gpt.html`

To view a test coverage report, go to:

Expand All @@ -143,13 +143,15 @@ A watch is also in place that will run continuous tests in the terminal as you e

## Contribute

Many SSPs, bidders, and publishers have contributed to this project. [20+ Bidders](https://github.com/prebid/Prebid.js/tree/master/src/adapters) are supported by Prebid.js.
Many SSPs, bidders, and publishers have contributed to this project. [60+ Bidders](https://github.com/prebid/Prebid.js/tree/master/src/adapters) are supported by Prebid.js.

Our PR review process can be found [here](https://github.com/prebid/Prebid.js/tree/master/pr_review.md).

### Add a Bidder Adapter

To add a bidder adapter, see the instructions in [How to add a bidder adaptor](http://prebid.org/dev-docs/bidder-adaptor.html).

Please **do NOT load Prebid.js inside your adapter**. If you do this, we will reject or remove your adapter as appropriate.
Please **do NOT load Prebid.js inside your adapter**. If you do this, we will reject or remove your adapter as appropriate.

### Code Quality

Expand All @@ -167,13 +169,13 @@ This will run tests and keep the Karma test browser open. If your `prebid.js` fi

+ For test results, see the console

+ To set breakpoints in source code, see the developer tools
+ To set breakpoints in source code, see the developer tools

Detailed code coverage reporting can be generated explicitly with

$ gulp test --coverage

The results will be in
The results will be in

./build/coverage

Expand All @@ -184,3 +186,6 @@ For instructions on writing tests for Prebid.js, see [Testing Prebid.js](http://
### Supported Browsers

Prebid.js is supported on IE9+ and modern browsers.

### Governance
Review our governance model [here Bidders](https://github.com/prebid/Prebid.js/tree/master/governance.md).
22 changes: 22 additions & 0 deletions governance.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
### Overview:

This document describes the governance model for the Prebid project. The Prebid project’s stated mission is to facilitate fair, transparent, and effective header bidding across the industry, and is responsible for creating and maintaining such projects as [Prebid.js](https://github.com/prebid/Prebid.js).

1. A single Tech Lead oversees the technical direction of the project and appoints Core Team members
2. The Core Team members maintain the project on an ongoing basis with direction from the Tech Lead.
3. In the event of any disagreements, the Tech Lead will make a final decision.
4. If there is no Tech Lead available to perform his/her duties, AppNexus Inc. will appoint one.

### Roles and Responsibilities:
- **User:** Any individual who consumes / uses the Prebid.js library.
- **Contributor:** Any individual who contributes code that is subsequently merged to the project. Contributed code is governed by the Prebid.js [license](https://github.com/prebid/Prebid.js/blob/master/LICENSE). Contributors are required to sign a CLA before any code can be committed (CLA pending).
- **Core Team Member:** An individual contributor who has been appointed by the Tech Lead on the project to maintain it and further it’s stated goals.
- **Tech Lead:** The Tech Lead is responsible for overall technical direction of the project. The Tech Lead will work closely with Core Team members to facilitate development and further the project goals.

### Current Prebid.js Core Team
- @mkendall07 (Tech Lead)
- @protonate
- @matthewlane
- @jaiminpanchal27
- @snapwich
- @harpere
23 changes: 23 additions & 0 deletions pr_review.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
## 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.

### General PR review Process
- Checkout the branch (these instructions are available on the github PR page as well).
- Verify PR is a single change type. Example, refactor OR bugfix. If more than 1 type, ask submitter to break out requests.
- Verify code under review has at least 80% unit test coverage. If legacy code has no unit test coverage, ask for unit tests to be included in the PR.
- Verify tests are green in Travis-ci + local build by running `gulp serve` | `gulp test`
- Verify no code quality violations are present from jscs (should be reported in terminal)
- Review for obvious errors or bad coding practice / use best judgement here.
- If the change is a new feature / change to core prebid.js - review the change with a Tech Lead on the project and make sure they agree with the nature of change.
- 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

### New Adapter or updates to adapter process
- Follow steps above for general review process. In addition, please verify the following:
- Verify that bidder has submitted valid bid params and that bids are being received.
- Verify that bidder is not manipulating the prebid.js auction in any way or doing things that go against the principles of the project. If unsure check with the Tech Lead.
- Verify that the bidder is being as efficient as possible, ideally not loading an external library, however if they do load a library it should be cached.
- 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.

0 comments on commit a9ebedf

Please sign in to comment.