Jump to: navigation, search

Difference between revisions of "TripleO/SpecReviews"

(Spec Review Process)
(Spec Review Process)
Line 5: Line 5:
 
Care should be taken when approving specs. An approved spec, and an associated blueprint, indicate that the proposed change has some priority for the TripleO project. We don't want a bunch of approved specs sitting out there that no community members are owning or working on. We also want to make sure that our specs and blueprints are easy to understand and have sufficient enough detail to effectively communicate the intent of the change. The more effective the communication, the more likely we are to elicit meaningful feedback from the wider community.
 
Care should be taken when approving specs. An approved spec, and an associated blueprint, indicate that the proposed change has some priority for the TripleO project. We don't want a bunch of approved specs sitting out there that no community members are owning or working on. We also want to make sure that our specs and blueprints are easy to understand and have sufficient enough detail to effectively communicate the intent of the change. The more effective the communication, the more likely we are to elicit meaningful feedback from the wider community.
  
To this end, we should be cognizant of the following checklist when approving specs.
+
To this end, we should be cognizant of the following checklist when reviewing and approving specs.
  
 
* Broad feedback from interested parties.
 
* Broad feedback from interested parties.
Line 33: Line 33:
 
* Reviewer workload
 
* Reviewer workload
 
** Specs turn into patches to codebases
 
** Specs turn into patches to codebases
** Have any reviewers volunteered to review patches that implement the spec?
+
** A +2 on a spec means that the core reviewer intends to review the patches associated with that spec in additional to their other core commitments for reviewer workload.
** Have any reviewers volunteered to go '''above and beyond''' their typical reviewer workload in order to review the relevant patches?
+
** A +1 on a spec from a core reviewer indicates that the core reviewer is not necessarily committing to review that specs patches.
 +
** Have any additional (perhaps non-core) reviewers volunteered to review patches that implement the spec?
 +
** There should be a sufficient number of core reviewers who have  volunteered to go '''above and beyond''' their typical reviewer workload in order to review the relevant patches. A sufficient number is dependent on the individual spec and the scope of the change.
 
** If reviewers have said they'll be reviewing a spec's patches '''instead''' of patches they'd review otherwise, that doesn't help much and is actually harmful to the overall project.
 
** If reviewers have said they'll be reviewing a spec's patches '''instead''' of patches they'd review otherwise, that doesn't help much and is actually harmful to the overall project.

Revision as of 12:41, 19 August 2014

THIS PAGE IS A DRAFT. DO NOT YET ASSUME ANY OF IT IS TRUE

Spec Review Process

Care should be taken when approving specs. An approved spec, and an associated blueprint, indicate that the proposed change has some priority for the TripleO project. We don't want a bunch of approved specs sitting out there that no community members are owning or working on. We also want to make sure that our specs and blueprints are easy to understand and have sufficient enough detail to effectively communicate the intent of the change. The more effective the communication, the more likely we are to elicit meaningful feedback from the wider community.

To this end, we should be cognizant of the following checklist when reviewing and approving specs.

  • Broad feedback from interested parties.
    • We should do our best to elicit feedback from operators, non-TripleO developers, end users, and the wider OpenStack community in general.
    • Mail the appropriate lists, such as opentack-operators and openstack-dev to ask for feedback. Respond to feedback on the list, but also encourage direct comments on the spec itself, as those will be easier for other spec reviewers to find.
  • Overall consensus
    • Check for a general consensus in the spec.
    • Do reviewers agree this change is meaningful for TripleO?
    • If they don't have a vested interest in the change, are they at least not objecting to the change?
  • Review older patchsets to make sure everything has been addressed
    • Have any reviewers raised objections in previous patchsets that were not addressed?
    • Have any potential pitfalls been pointed out that have not been addressed?
  • Impact/Security
    • Ensure that the various Impact (end user, deployer, etc) and Security sections in the spec have some content.
    • These aren't sections to just gloss off over after understanding the implementation and proposed change. They are actually the most important sections.
    • It would be nice if that content had elicited some feedback. If it didn't, that's a probably a good sign that the author and/or reviewers have not yet thought about these sections carefully.
  • Ease of understandability
    • The spec should be easy to understand for those reviewers who are familiar with the project. While the implementation may contain technical details that not everyone will grasp, the overall proposed change should be able to be understood by folks generally familiar with TripleO. Someone who is generally familiar with TripleO is likely someone who has run through devtest, perhaps contributed some code, or participated in reviews.
    • To aid in comprehension, grammar nits should generally be corrected when they have been pointed out. Be aware though that even nits can cause disagreements, as folks pointing out nits may be wrong themselves. Do not bikeshed over solving disagreements on nits.
  • Implementation
    • Does the implementation make sense?
    • Are there alternative implementations, perhaps easier ones, and if so, have those been listed in the Alternatives section?
    • Are reasons for discounting the Alternatives listed in the spec?
  • Ownership
    • Is the spec author the primary assignee?
    • If not, has the primary assignee reviewed the spec, or at least commented that they agree that they are the primary assignee?
  • Reviewer workload
    • Specs turn into patches to codebases
    • A +2 on a spec means that the core reviewer intends to review the patches associated with that spec in additional to their other core commitments for reviewer workload.
    • A +1 on a spec from a core reviewer indicates that the core reviewer is not necessarily committing to review that specs patches.
    • Have any additional (perhaps non-core) reviewers volunteered to review patches that implement the spec?
    • There should be a sufficient number of core reviewers who have volunteered to go above and beyond their typical reviewer workload in order to review the relevant patches. A sufficient number is dependent on the individual spec and the scope of the change.
    • If reviewers have said they'll be reviewing a spec's patches instead of patches they'd review otherwise, that doesn't help much and is actually harmful to the overall project.