Difference between revisions of "StarlingX/CodeSubmissionGuidelines"
Ghada.khalil (talk | contribs) |
|||
Line 1: | Line 1: | ||
= StarlingX Code Submission Guidelines = | = StarlingX Code Submission Guidelines = | ||
− | * Use Gerrit for StarlingX code reviews | + | * '''Use Gerrit''' for StarlingX code reviews |
− | * Follow the Openstack Git Commit Good Practice [[GitCommitMessages|here]] | + | ** Follow the Openstack Git Commit Good Practice [[GitCommitMessages|here]] |
− | * Add the core reviewers for the affected sub-project to the review | + | * '''Add the core reviewers''' for the affected sub-project to the review |
** The core reviewers are listed on each sub-project wiki pages. The list of sub-projects is available [[StarlingX#StarlingX_Projects|here]] | ** The core reviewers are listed on each sub-project wiki pages. The list of sub-projects is available [[StarlingX#StarlingX_Projects|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 |
− | * Link your | + | *** 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. |
− | ** For traceability, always link your code change to a story or bug. Gerrit will update the status of the story/bug automatically once the code is merged. | + | ** 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''' | ||
+ | ** 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 StoryBoard Stories: Specify the story and task ID in the commit message as follows: | ||
*** Story: $story_id | *** Story: $story_id | ||
Line 19: | Line 22: | ||
*** If a fix requires multiple commits, use "Partial-Bug" with only the final commit using "Closes-Bug" | *** If a fix requires multiple commits, use "Partial-Bug" with only the final commit using "Closes-Bug" | ||
*** Example: https://review.openstack.org/596305 | *** 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. |
− | ** | + | ** At a minimum, make sure the new code compiles and builds successfully. |
− | * Early | + | ** 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. |
− | ** Such changes should be marked as WIP in the commit message and given a Workflow -1 by the | + | ** 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 code reviewers to ask about testing details as part of the gerrit code inspection, so be prepared. | ||
+ | * '''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. | ** 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. |
Revision as of 22:03, 25 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
- 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.
- 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:
- Story: $story_id
- Task: $task_id
- Example: https://review.openstack.org/#/c/590083/
- 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.
- At a minimum, 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 code reviewers to ask about testing details as part of the gerrit code inspection, so be prepared.
- 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.