Skip to content

Latest commit

 

History

History
55 lines (45 loc) · 3.43 KB

pr_review.asciidoc

File metadata and controls

55 lines (45 loc) · 3.43 KB
Raw

Reviewing Pull Requests

Table of Contents

This document provide a guideance for the Github Pull Request review. It’s not a complete list and can be little bit overhelming for smaller patches, however it sets the standard we should all aim for.

How to use the list

It’s a good idea to use Saved reply Github feature and use it for patches which require extra attention.

Review list

  • ❏ Ticket number, title and description is correct.

  • ❏ Fixes the problem described in the issue.

  • ❏ No unrelated changes present.

  • ❏ Redmine opened for the correct project.

  • ❏ No use of .to_sym, .send or other reflection on untrusted inputs.

  • ❏ All strings extracted (use :mark_translated: true).

  • ❏ All string extractions follow our rules.

  • ❏ Appropriate permissions for non-admin users added.

  • ❏ New code hasn’t been copy-pasted from elsewhere.

  • ❏ All new AR fields have appropriate validators.

  • ❏ No exceptions are swallowed/squashed.

  • ❏ New exceptions are type of Foreman::Exception or WrappedException.

  • ❏ Covered with meaningful unit/functional/integration test(s).

  • ❏ New view templates have .html.erb extensions, not just .erb.

  • ❏ Mixins use ActiveSupport::Concern instead of class_eval etc.

  • ❏ Concerns and new classes are added to app/ and not lib/.

  • ❏ Apipie documentation is correct: HTTP methods, names of parameters, required true/false.

  • ❏ Scoped_search definitions are added for new model attributes where appropriate.

  • ❏ Scoped_search definitions for virtual fields (:ext_method) use :only_explicit.

  • ❏ Rails logging is appropriately used. Enough debug log statements.

  • ❏ Block used for logger.debug to lazily evaluate expensive debug statements.

  • ❏ Deprecations use Foreman::Deprecation with a deadline of latest stable + three versions.

  • ❏ New controllers/actions have API counterparts.

  • ❏ New controllers/actions have Hammer CLI counterparts.

  • ❏ Public HTTP API not changed in incompatible way.

  • ❏ No memory or performance concerns.

  • ❏ Necessary packaging done.

  • ❏ Agreed who would provide user documentation.

  • ❏ Agreed who would provide community demo.

  • ❏ Agreed who would provide Upgrade Notes or New Mainline Features docs.

Feel free to edit this page and more items to the list.

Additional info

String extractions have several hard to remember rules.

Scoped_search definitions for virtual fields can stop free text search if the field definition added doesn’t actually exist - i.e. it’s a fake field that uses :ext_method to return results. In this case it should use :only_explicit ⇒ true too so it isn’t searched in free text searches (see also #5777 for another instance of this).

For internal APIs etc that are changing, use Foreman::Deprecation.deprecation_warning to add a warning for plugins that are triggering old codepaths. We ship deprecated methods for two stable releases, giving plugin authors the chance to ship their plugin with compatibility for a couple of versions before forcing their hand. If our most recent stable release is say, 1.9-stable, then the deprecation first goes into develop which becomes 1.10, still in 1.11 and so the deadline would be 1.12. It would be removed after 1.11-stable’s branched. Use the most recent stable number, plus three to work out the deadline.