Jump to: navigation, search

Difference between revisions of "StarlingX/CodeSubmissionGuidelines"

m
m
Line 7: Line 7:
 
** In order for code to get merged, two core reviewers must give the review +2. A final Workflow +1 from one core reviewer will allow the code to merge
 
** In order for code to get merged, two core reviewers must give the review +2. A final Workflow +1 from one core reviewer will allow the code to merge
 
** Authors should not +2 or W+ their own code submissions
 
** Authors should not +2 or W+ their own code submissions
*** If an exception is needed (ex: emergency fix for a broken build), the author should send an email to the mailing list to let the community know.
+
*** If an exception is needed (ex: emergency fix for a broken build), the author should send an email to the mailing list to let the community know or contact the core reviewers on IRC.
 
** For more details on the code review process, refer to the [https://docs.openstack.org/infra/manual/developers.html#code-review Openstack Developer Guide]
 
** For more details on the code review process, refer to the [https://docs.openstack.org/infra/manual/developers.html#code-review Openstack Developer Guide]
 
* '''Link your review to a StoryBoard Story or Launchpad Bug'''
 
* '''Link your review to a StoryBoard Story or Launchpad Bug'''

Revision as of 12:57, 26 October 2018

StarlingX Code Submission Guidelines

  • Use Gerrit for StarlingX code reviews
    • Follow the Openstack Git Commit Good Practice here
  • Add the core reviewers for the affected sub-project to the review as well as any other interested reviewers
    • The core reviewers are listed on each sub-project wiki pages. The list of sub-projects is available here
    • In order for code to get merged, two core reviewers must give the review +2. A final Workflow +1 from one core reviewer will allow the code to merge
    • Authors should not +2 or W+ their own code submissions
      • If an exception is needed (ex: emergency fix for a broken build), the author should send an email to the mailing list to let the community know or contact the core reviewers on IRC.
    • For more details on the code review process, refer to the Openstack Developer Guide
  • Link your review to a StoryBoard Story or Launchpad Bug
    • For traceability, always link your code change to a story or bug. The story/bug will give reviewers context for the code changes.
    • Gerrit will update the status of the story/bug automatically once the code is merged.
    • Linking to StoryBoard Stories: Specify the story and task ID in the commit message as follows:
    • Linking to Launchpad Bugs: Specify the Bug ID in the commit message as follows:
      • Closes-Bug: $bug_id -- use 'Closes-Bug' if the commit is intended to fully fix and close the bug being referenced.
      • Partial-Bug: $bug_id -- use 'Partial-Bug' if the commit is only a partial fix and more work is needed.
      • Related-Bug: $bug_id -- use 'Related-Bug' if the commit is merely related to the referenced bug.
      • If a fix requires multiple commits, use "Partial-Bug" with only the final commit using "Closes-Bug"
      • Example: https://review.openstack.org/596305
  • Pre-Review / Pre-Submission Testing
    • For the majority of cases, it is expected that the author completes their testing before posting a review.
    • Make sure the new code compiles and builds successfully.
    • Run tox tests (flake8, py27, etc) successfully. These can all be run manually prior to launching a review.
    • Update existing automated unit tests and add new ones when applicable.
    • Verify basic functional testing on a live system to ensure the new code gets executed and functions correctly.
    • If needed, consult with the core reviewers or send questions to the mailing list regarding required/recommended testing.
    • It's customary for core reviewers to ask about testing details as part of the gerrit code inspection. Adding the details of the testing as a comment in the review will speed up the process.
  • Early Review / Feedback
    • In specific cases, changes can be posted for early review prior to testing (ex: need early feedback on detailed design/coding approach)
    • Such changes should be marked as WIP in the commit message and given a Workflow -1 immediately by the author
    • The author should also include a comment in the review explaining the purpose of the review and why the testing is deferred.
    • Reviewing code early and often helps catch design and coding errors sooner and shows us following the Four Opens.
  • Cherry-picking
    • All code changes must be pushed to master first and then cherry-picked to the appropriate release branch as needed
    • Exception: Feature branches used during development
  • Patch Rebase
    • During patch re-base, there is a chance that patches can be applied by treating the patch line numbers as approximate, rather than a strict requirement, just so long as the before/after context seems to be correct. They require fuzzing during the patch apply, and an .orig files will be created as the consequence of applying patches that are not clean.
    • In StarlingX, we will not accept fuzzing patches. All patches are required to be re-based cleanly so that no fuzzing and no .orig files are generated.