Jump to: navigation, search

Difference between revisions of "Magnum/Contributing"

(Advice for Reviewers)
 
(2 intermediate revisions by one other user not shown)
Line 1: Line 1:
__TOC__
+
Please refer to the latest documentation:
 +
* https://docs.openstack.org/magnum/latest/contributor/index.html
  
 
= Before You Begin =
 
= Before You Begin =
Line 195: Line 196:
 
* If a patch submitted by one contributor is picked up and completed by another contributor, [http://www.mail-archive.com/openstack-dev@lists.openstack.org/msg05998.html special handling] of the resolution should be used.
 
* If a patch submitted by one contributor is picked up and completed by another contributor, [http://www.mail-archive.com/openstack-dev@lists.openstack.org/msg05998.html special handling] of the resolution should be used.
 
=== Advice and Requirements for Reviewers ===
 
=== Advice and Requirements for Reviewers ===
* Reviewers shall align their actions with the [https://www.openstack.org/legal/community-code-of-conduct/ OpenStack Community Code of Conduct].
+
We are a team, and it's extremely important that we treat each other with respect and courtesy. Reviewers shall align their actions with the [https://www.openstack.org/legal/community-code-of-conduct/ OpenStack Community Code of Conduct].  
* A <code>-1</code> vote is an opportunity to make our code better before it is merged. Please do your best to make helpful, actionable <code>-1</code> votes.  
+
 
* Avoid the temptation to blindly <code>+1</code> code without reviewing it in sufficient detail to form an opinion.
+
The following workflow states may by applied to each review:
* When voting <code>-1</code> on a patch, it means that you want the submitter to make a revision in accordance with your feedback before core reviewers should consider this code for merge.
+
 
* If you ask a question, you should vote <code>0</code> unless you anticipate that the answer to that question is likely to cause you to vote against the patch without further revisions.
+
{| class="wikitable"
* If you use a <code>-1</code> vote for a question, and the contributor answers the question, please respond acknowledging the question. Either change your vote or follow up with additional rationale for why this should remain a <code>-1</code> comment.
+
|-
* A <code>-2</code> vote is a veto by a single core reviewer. It is sticky. That means that even if you revise your patch, that vote will persist. To allow your patch to merge, that same reviewer must clear the <code>-2</code> vote first. This vote is used when you have contributed something that is not in alignment with the current project vision, or is implemented in a way that can not be accepted. For example, security concerns that a core reviewer wants to individually re-evaluate before allowing the contribution to continue. It can also be used as a way to halt further gate testing of a patch, if something is included that may break the gate. It works even after a <code>2*+2,+A</code> approval for merge, but before the patch reaches MERGED state.  
+
| '''State''' || '''Meaning''' || '''Detail'''
* A reviewer casting a <code>-2</code> vote shall include a review comment that clearly states what can be changed by the contributor to result in removal of the <code>-2</code>. Use caution applying this vote, because contributors perceive this action as hostile unless it is accompanied with a genuine offer to help remedy the concern collaboratively.
+
|-
* To avoid a <code>-2</code> vote, discuss your plans with the development team prior to writing code, and post a WIP (workflow-1) patch while you are working on it, and ask for input before you submit it for merge review.
+
| <code>workflow-1</code> || Work in progress || This patch is submitted for team input, but should not yet be considered for merge.
 +
|-
 +
| <code>workflow-0</code> || Ready for reviews || This patch should be considered for merge.
 +
|-
 +
| <code>workflow+1</code> || Approved || This patch has received at least two +2 votes, and is approved for merge.
 +
|-
 +
|}
 +
 
 +
The following votes may be applied to a review.
 +
 
 +
{| class="wikitable"
 +
|-
 +
| '''Vote''' || '''Meaning''' || '''Detail'''
 +
|-
 +
| <code>-2</code> || Do Not Merge ||
 +
* <code>WARNING</code>: Use extreme caution applying this vote, because contributors perceive this action as hostile unless it is accompanied with a genuine offer to help remedy a critical concern collaboratively.
 +
* This vote is a veto that indicates a critical problem with the contribution. It is ''sticky'', meaning it must be removed by the individual who added it, even if further revisions are made.
 +
* All <code>-2</code> votes shall be accompanied with a polite comment that clearly states what can be changed by the contributor to result in reversal of the vote.  
 +
* Core reviewers may use this vote:
 +
** In an emergency to prevent breaking the gate. This vote works to halt a merge even after a <code>2*+2,+A</code> approval, but before the patch reaches MERGED state.
 +
** To indicate a critical problem to address, such as a security vulnerability that other core reviewers may be unable to recognize.
 +
* The PTL may use this vote:
 +
** To indicate a decision that the patch is not consistent with the direction of the project.
 +
** While coordinating a release to prevent incompatible changes from merging before the release is tagged.
 +
** To address a critical concern with the contribution.
 +
* To avoid a <code>-2</code> vote on your contribution, discuss your plans with the development team prior to writing code, and post a WIP (workflow-1) patch while you are working on it, and ask for input before you submit it for merge review.
 +
|-
 +
| <code>-1</code> || This patch needs further work before it can be merged ||
 +
* This vote indicates an opportunity to make our code better before it is merged.
 +
* It asks the submitter to make a revision in accordance with your feedback before core reviewers should consider this code for merge.  
 +
* This vote shall be accompanied with constructive and actionable feedback for ''how'' to improve the submission.
 +
* If you use a <code>-1</code> vote to ask a question, and the contributor answers the question, please respond acknowledging the answer. Either change your vote or follow up with additional rationale for why this should remain a <code>-1</code> comment.
 +
* These votes will be cleared when you make a revision to a patch set, and resubmit it for review.
 +
|-
 +
| <code>0</code> || No Score ||
 +
* Used to make remarks or ask questions that may not require a revision to answer.
 +
|-
 +
| <code>+1</code> || Looks good to me, but someone else must approve ||
 +
* Used to validate the quality of a contribution and express agreement with the implementation.
 +
* Resist the temptation to blindly <code>+1</code> code without reviewing it in sufficient detail to form an opinion.
 +
|-
 +
| <code>+2</code> || Looks good to me (core reviewer) ||
 +
* Used by core reviewers to indicate acceptance of the patch in its current form.  
 +
* Two of these votes are required for <code>+A</code>.
 +
|-
 +
| <code>+A</code> || Approval for Merge ||
 +
* This means setting the <code>workflow+1</code> state, and is typically added together with the final <code>+2</code> vote.
 +
* The approving reviewer is responsible for verifying that all open questions and concerns have been adequately addressed prior to taking this action.
 +
|-
 +
|}
 +
 
 +
If these guidelines and requirements are not clear, or if you have suggestions for how to improve them, please contact the PTL for help.

Latest revision as of 09:52, 15 January 2018

Please refer to the latest documentation:

Before You Begin

To familiarize yourself with Magnum, try it out using the information in our Dev Quickstart Guide and Manually Adding Magnum to DevStack Guide. When you are ready to start contributing, you will need to execute an OpenStack CLA. This is required before you can submit reviews to our Magnum Repo. For information about how prepare for contribution, please consult the How To Contribute wiki page.

We strongly recommend that you spend 40 minutes watching the 7 Habits of Highly Effective Contributors video. This is Adrian Otto's philosophy on the behaviors that you should exhibit to become a valued member of our contributor community. Adrian is the founding PTL of Magnum.

Sign Up for the OpenStack Development Mailing List

Head on over to OpenStack Dev Mailing List, fill out the fields under "Subscribing to OpenStack-dev" and click Subscribe.

Learn About Gerrit

Be sure to read the Developer's Guide for information about how to submit your commit for review so it can be merged into the Magnum code base.

If you do not have a gerrit username setup or do not remember it, verify and configure it here: https://review.openstack.org/#/settings/

Keep that handy for the "git review -s" command later in the https://wiki.openstack.org/wiki/Magnum/Contributing#Setting_up_your_git_review_settings and https://wiki.openstack.org/wiki/Magnum/Contributing#Set_up_your_local_branch sections below...

You'll also need to upload an SSH public key, specifically for Gerrit: https://review.openstack.org/#/settings/ssh-keys

Setting up your git review settings

New to git? Try the 15-minute high-level interactive Git demo GitHub Demo

Need a git refresher? Download a git reference direct from the source: GitHub Git Cheat Sheet

Then:

 git config --global user.name "Firstname Lastname"
 git config --global user.email "your_email@youremail.com"

In the event your launchpad and gerrit usernames are identical:

 git config --global gitreview.username "your_launchpad_username"

In the event your launchpad and gerrit usernames are not identical:

 git config --global gitreview.username "your_gerrit_username"

To check your git configuration:

 git config --list

Installing git-review

On Ubuntu, MacOSx, or most other Unix-like systems, it is as simple as:

 pip install git-review

There are other installation options detailed in the Developer's Guide. You can now check out the Magnum code and begin working on it:

Your first commit

Set up your local branch

 git clone git://git.openstack.org/openstack/magnum
 cd magnum
 git checkout -b [branch name (see below...)]
 git review -s

To find out what branch names are valid, try out one of the useful git branch commands here: http://gitready.com/intermediate/2009/02/13/list-remote-branches.html

If you receive the following error when running the "git review -s" command:

 We don't know where your gerrit is. Please manually create a remote
 named "gerrit" and try again.

Perform the following (substituting <your gerrit username> with your specific gerrit username...):

 git remote add gerrit ssh://<your gerrit username>@review.openstack.org:29418/openstack/magnum.git
 git review -s

If you receive the following error when running the "git review -s" command:

 Agent admitted failure to sign using the key

See this support page: https://help.github.com/articles/error-agent-admitted-failure-to-sign/

Identify bugs

Observe a bug? Report it here: https://launchpad.net/magnum

  1. Click the Report a Bug button.
  2. Enter a Summary in the field and click Next. Here's an example: Stale link to Gerrit Workflow
  3. Review the choices you've been given. If you spot your bug, click on the bug and review its status; proceed to the awesome code section below if there's more to do.. If you do not spot your bug, proceed to the next step.
  4. Click the No, I need to report a new bug button.
  5. Type in a detailed description of the bug in the "Further information" field and click Submit bug report. Here's an example description for our previous example: In doc/source/dev/quickstart.rst the link to the Gerrit workflow is incorrect. It should be: http://docs.openstack.org/infra/manual/developers.html

NOTE: If you are fixing something trivial, that is not actually a functional defect in the software, you can do that without filing a bug ticket, if you don't want it to be tracked when we tally this work between releases. If you do this, just mention it in the commit message that it's a trivial change that does not require a bug ticket. You can reference this guideline if it comes up in discussion during the review process. Functional defects should be tracked in bug tickets. New features should be tracked in blueprints. Trivial features may be tracked using a bug ticket marked as 'Wishlist' importance.

Write some awesome code

At this point can write your code and push it to Gerrit for review.

 git add <list of files you added/changed>


If you submitted a bug in the previous section, be sure to add the bug link in the commit message.

Each commit line should conform with the OpenStack message structure guidelines found at https://wiki.openstack.org/wiki/GitCommitMessages#Summary_of_Git_commit_message_structure

Highlights include:

  • There should be no whitespace at the end of the lines
  • The order of the lines is important
    • The first line of the commit message should be <=50 characters
    • The next line is blank
    • The next section is a verbose description of the commit (lines must be <= 72 characters)
    • The next line is blank
    • The next lines contain the bug number or the blueprint name and possibly other information:
      • Closes-Bug/Partial-Bug/Related-Bug: hash symbol (#) followed by a bug number (e.g. Closes-Bug: #111111)
      • Blueprint: blueprint name (e.g., "Implements: blueprint libvirt-xml-cpu-model")
    • Once the change is submitted with git review a "Change-Id:" line will be appended with an identifier generated by Gerrit


An example of a commit message:

 Adjust Gerrit workflow Link
 <blank_line>
 The Gerrit workflow link in the previous version is no longer valid.
 This patch updates documentation to the current link.
 <blank_line>
 Closes-Bug: #<launchpad_bug_number>
 Change-Id: <generated_by_gerrit>


This is but one example. You can find more detailed information about commit messages at https://wiki.openstack.org/wiki/GitCommitMessages


Once you organize the above information, perform the following command to type it in:

 git commit -a


Then, submit the review:

 git review -v --draft


Once you are happy with your code and want it to be reviewed you want to convert it from a Draft. "Sign In" at https://review.openstack.org/ and after verifying the review yourself hit the "Publish" button on the page.

If you know you are ready for others to review your code, you can skip the draft step and just do:

git review -v


By this point, your code will be reviewed by the Openstack automatic testing frameworks and, potentially, your peers on the project.

If you want to revise your patchset in the review system in response to feedback, first navigate back to your development area:

 cd ~/magnum [or wherever that is for your setup...]


Download the change. Be sure to use the review number and NOT the bug number:

 git review -d <review number>


Then, make your changes, and update the commit message using the aforementioned commit guidelines:

git commit --amend


Then:

git review -v


Upon approval of the review your code will be automatically merged.

Need somewhere to start writing code?

You can also contribute to the quality of the code by looking at the coverage of current Magnum unit tests and improving the code coverage. You can review the code coverage of your currently checked out code with:

 $ tox -e cover

This will produce a cover directory that provides HTML output showing the code coverage as well as the specific lines of code that are not covered.

Low Hanging Fruit

Some bugs have been identified by the bug manager as appropriate for first time contributors. These bugs should require small patches, and don't require an extensive understanding of Magnum to work on. Find them tagged with the low-hanging-fruit tag in the bug system.

How to Resolve Merge Conflicts

If another patch changes the same line of code as your patch, and it gets merged before yours, your patch may get a patch in merge conflict status in red. If that happens, here is how you can resolve it. You will need the patch number from the Gerrit URL https://review.openstack.org/NNNNN to make your rebase a revision of the previous review.

git clone https://github.com/openstack/magnum
cd magnum
git review -d NNNNN
git rebase master

Now edit all files with merge conflicts to resolve them by making the code look the way it should both with the recently merged code, and your own. Then git add each of the files you edited.

git add path/to/file/you/edited
git rebase --continue
git review -v

That will resubmit your patch with the original commit message, and the rebased code with your merge fix as a new patch set.

Reviews

The OpenStack CI system uses the concept of core reviewers. These are individuals who have consistently reviewed code for the project, and helped over a considerable period of time to improve the quality and consistency of what we merge into the code base. Project contributors will feel that this reviewer is a positive influence on the team and that they maintain the values and traditions of the OpenStack development community.

Policies

Existing core reviewers may nominate new ones in an ML thread. Consent among the current reviewers shall result in the declaration of the new core reviewer by the PTL. Lack of unanimous consent shall be carefully considered, and a final decision informed by input from from active team members shall be made by the PTL. Core reviewers who are judged by their peers in the core review group to fall short of the expectations for contribution of a core reviewer may be nominated for return to regular reviewer status.

Patches require a core reviewer to mark a review as "Approved" before they are merged.

Review Guidelines

Code Approval for Merge

  • For Approval, two core reviewers shall supply a +2.

Continuing Someone Else's Contribution

  • If a patch submitted by one contributor is picked up and completed by another contributor, special handling of the resolution should be used.

Advice and Requirements for Reviewers

We are a team, and it's extremely important that we treat each other with respect and courtesy. Reviewers shall align their actions with the OpenStack Community Code of Conduct.

The following workflow states may by applied to each review:

State Meaning Detail
workflow-1 Work in progress This patch is submitted for team input, but should not yet be considered for merge.
workflow-0 Ready for reviews This patch should be considered for merge.
workflow+1 Approved This patch has received at least two +2 votes, and is approved for merge.

The following votes may be applied to a review.

Vote Meaning Detail
-2 Do Not Merge
  • WARNING: Use extreme caution applying this vote, because contributors perceive this action as hostile unless it is accompanied with a genuine offer to help remedy a critical concern collaboratively.
  • This vote is a veto that indicates a critical problem with the contribution. It is sticky, meaning it must be removed by the individual who added it, even if further revisions are made.
  • All -2 votes shall be accompanied with a polite comment that clearly states what can be changed by the contributor to result in reversal of the vote.
  • Core reviewers may use this vote:
    • In an emergency to prevent breaking the gate. This vote works to halt a merge even after a 2*+2,+A approval, but before the patch reaches MERGED state.
    • To indicate a critical problem to address, such as a security vulnerability that other core reviewers may be unable to recognize.
  • The PTL may use this vote:
    • To indicate a decision that the patch is not consistent with the direction of the project.
    • While coordinating a release to prevent incompatible changes from merging before the release is tagged.
    • To address a critical concern with the contribution.
  • To avoid a -2 vote on your contribution, discuss your plans with the development team prior to writing code, and post a WIP (workflow-1) patch while you are working on it, and ask for input before you submit it for merge review.
-1 This patch needs further work before it can be merged
  • This vote indicates an opportunity to make our code better before it is merged.
  • It asks the submitter to make a revision in accordance with your feedback before core reviewers should consider this code for merge.
  • This vote shall be accompanied with constructive and actionable feedback for how to improve the submission.
  • If you use a -1 vote to ask a question, and the contributor answers the question, please respond acknowledging the answer. Either change your vote or follow up with additional rationale for why this should remain a -1 comment.
  • These votes will be cleared when you make a revision to a patch set, and resubmit it for review.
0 No Score
  • Used to make remarks or ask questions that may not require a revision to answer.
+1 Looks good to me, but someone else must approve
  • Used to validate the quality of a contribution and express agreement with the implementation.
  • Resist the temptation to blindly +1 code without reviewing it in sufficient detail to form an opinion.
+2 Looks good to me (core reviewer)
  • Used by core reviewers to indicate acceptance of the patch in its current form.
  • Two of these votes are required for +A.
+A Approval for Merge
  • This means setting the workflow+1 state, and is typically added together with the final +2 vote.
  • The approving reviewer is responsible for verifying that all open questions and concerns have been adequately addressed prior to taking this action.

If these guidelines and requirements are not clear, or if you have suggestions for how to improve them, please contact the PTL for help.