-
Notifications
You must be signed in to change notification settings - Fork 265
Reviewing Pull Requests
All changes to the BrowserID repository should be made via pull requests. Review and merging of pull requests can be performed by any single committer to the BrowserID repo, but additional review might be required when the change has UX, security, or operational implications.
Backend unit tests, frontend unit tests, and automated browser tests should run across all supported browsers. If the code introduces new behaviors that can be tested in an automated fashion, but are not, ask that tests are added to the pull request.
If any new processes are added to a running deployment, this will require operational attention and review. Addition of new processes should be preceeded by a short document which describes what we're changing and why. Addition of new outbound network requests will require manipulation of firewall permissions. Addition of native libraries will require that we have these installed in all of our environments.
Any changes of this nature should be reviewed by ops and called out in the ChangeLog explicitly as part of the pull request.
Changes inside lib/db/mysql.js might indicate database schema changes. If changes were made to the schema, additional review should be sought to ensure our database architecture remains scalable and optimal. Also, the commit should come with an addition to the current ChangeLog to ensure that people responsible for making the changes are aware.
If new user facing strings were introduced in the change, the reviewer should ensure that they are properly contained inside a gettext() function invocation, and that the strings are properly extracted (see scripts/extract-po.sh for the paths that are searched for strings)
If changes are made to the system's APIs (under lib/wsapi) or involve any libraries that are used for encryption or authentication, additional security review should be sought. Also, if there are any changes to cross iframe communication (changes in winchan.js, jschannel.js, include,js, or dialog.js could have security implications).
Accidental or intentional changes to the existing API could have significant impact on users. Any changes to include.js, provisioning_api.js, authentication_api.js or our data formats should be given extra thought to ensure we don't break existing users of Persona.
- Does pull request do what it says it does?
- Does the code make sense? Can you understand it on a localized and global level?
- Does the code belong where it is? Is there a better place to put it?
- Does the code fit overall style?
- Does it need more/better comments?
- Are the function and variable names descriptive?
- Are boundary cases considered?
- Are error cases handled?
If changes need to be made, write your feedback and make sure to be descriptive. Feel free to use line level comments to get feedback or ask specific questions about the implementation.
If additional review is required for one of the areas above, assign the pull request to the appropriate person, and ask in irc.mozilla.org#identity if you don't know who that is.
If the code meets your expected standards, mark it an r+ and click "Merge".
Once the merge is complete, the merger has the responsibility to delete the branch from origin.