Jump to: navigation, search

Difference between revisions of "Your first review on Zaqar"

m (Kgriffs moved page Marconi/docs/contrib/tutorials/review to Your First Review (Marconi): Follow mediawiki naming conventions)
 
(16 intermediate revisions by 3 users not shown)
Line 1: Line 1:
 +
=== Reviewing and being reviewed ===
  
===Resources===
+
The review stage is a very important part in the development process. There are many reasons to say this:
The [https://wiki.openstack.org/wiki/Gerrit_Workflow Gerrit Workflow] page should be your main resource for how to submit a patch. It covers creating the necessary accounts and signing the necessary agreements required for you to submit your patch. It also covers how you should set up your git environment and what your commit messages should look like.
 
  
===Agreements and Accounts===
+
* Getting other developers feedback minimizes the risk of adding regressions to the code base and ensures the quality of the code being merged.
Folllow the [https://wiki.openstack.org/wiki/Gerrit_Workflow#Account_Setup Account Setup] instructions to get all your accounts in order. You will need a launchpad and and openstack account.
+
* It helps to build community. Everyone appreciate when their code is reviewed, and of course they will review your code when they have their chance.
 +
* It also helps developers to improve, since they are always learning from other's points of view.
 +
* Its a great practice to get familiar with the code.
  
===Branches and Commits===
 
The [https://wiki.openstack.org/wiki/Gerrit_Workflow#Project_Setup Gerrit Workflow] page contains all  you need to know about setting up your repository for Gerrit. Follow the instructions carefully so you don't miss anything.
 
  
====Common Problems====
+
Everyone is encouraged to review code! It's not necessary that you know every detail of the code base, you only have to understand what the code related to the
1. You realized that you were working in master and you HAVEN'T made any commits.
+
fix does.
Solution
 
<pre>
 
git checkout -b newbranch    #if you already created the branch, omit the -b
 
git commit -a -m "Edited"
 
</pre>
 
Now all your changes are in newbranch. Problem solved!
 
  
2. You realized that you were working in master and you HAVE made commits to master
+
=== Step by step ===
<pre>
 
git branch newbranch
 
git reset --hard HEAD~x    #x is the number of commits you have made to master. YOU WILL LOSE ANY UNCOMMITTED WORK
 
git checkout newbranch
 
</pre>
 
Your commits are now in newbranch. Problem solved!
 
  
3. You made multiple commits and realized that Gerrit needs one commit per patch
+
* Go to review.openstack.org and filter by [https://review.openstack.org/#/q/status:open+project:openstack/zaqar,n,z Open Zaqar fixes].
You need to squash your previous commits. Make sure you are in your branch and follow [http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html this] guide.  
+
* Select a fix from the list to review.
Fill in the commit message as specified on the [https://wiki.openstack.org/wiki/Gerrit_Workflow#Committing_Changes Gerrit Workflow] page
+
* Download the patch to your local repository and test it.
  
===Git Review===
+
 
Once you have all your changes and commits looking nice and pretty, you are ready to submit your patch. Follow [https://wiki.openstack.org/wiki/Gerrit_Workflow#Committing_Changes these instructions] to submit your code for review.
+
<pre><nowiki>
 +
git review -d [review-id]
 +
</nowiki></pre>
 +
 
 +
The review-id is located the number in the URL (check the screenshot for more details).
 +
 
 +
 
 +
Example
 +
 
 +
<pre><nowiki>
 +
git review -d 92979
 +
</nowiki></pre>
 +
 
 +
 
 +
[[File:Marconi_Review_ID.png|Medium|Download the patch using the Review ID in the address bar.]]
 +
 
 +
This will create a branch with the author name and will allow you to test the patch in your local environment.
 +
* Inspect the code. Is important that you consider all the good programming practices you know here. Take a look to the '''checklist''' for some ideas.
 +
* If during the inspection you see a specific line you would like to bring up to discussion in the final review, leave your inline comment. This will make the review process easier. As you compose your comments, please keep in mind [[Reviewer_Guide_(Marconi)|these guidelines]].
 +
* Hit the "Review" button in the web UI.
 +
* Time to score!
 +
** When to score with -1? If you think that there are things that should be fixed, then voting -1 is ok. We have to be careful with this since we don't want to stall the cycle just because a few nits, so downvoting also depends on the current stage of the development cycle and the severity of the flaw you see.
 +
** When to score with 0? If you are the author of the fix and you want respond to the reviewers comments, or if you are a reviewer and you want to point out some reminder for future developing (e.g. the deadline is the next day and the fix needs to be merged, but you want something to be improved).
 +
** When to score with +1? If the fix works and you think that the code looks good, upvoting is your choice.
 +
* Remember to leave any comment you think is important in the comment form, and when you are done just click "Publish Comments".
 +
 
 +
 
 +
For more details on how to do a review, check out the [http://docs.openstack.org/infra/manual/developers.html#code-review review] section in the [http://docs.openstack.org/infra/manual/developers.html Developer's Guide].
 +
 
 +
=== Checklist ===
 +
 
 +
* '''No idea on how to start?''' Take a moment to check different reviews for patches in review.openstack.org. Don't forget about inline comments! Also, try to select an easy patch for your first review. That will help you to gain some confidence.
 +
* If you already submitted a patch, take a moment to think about the reviews you received. '''Which were the most useful comments?''' Consider the feedback that helped you the most in the past and try to do the same for the fix you are reviewing. Helpful review comments will be appreciated by the developer that is submitting the fix.
 +
* Code location feedback. '''Do you consider that some code should be better located in another place within the file, or maybe in another file?''' If so, suggest it the review comment and score with -1 if you think that it's that important.
 +
* Code style feedback. '''Do you think that the code structure could be improved?''' Keep the DRY, YAGNI and KISS principles in mind.
 +
* Grammar/orthography feedback. Many of our contributors are not native English speakers, so its common to find some of this errors. Git Commit messages are as important as code, so you should review it too. Check [[GitCommitMessages]] for more information on how a Git Commit message should be written.

Latest revision as of 18:32, 5 December 2014

Reviewing and being reviewed

The review stage is a very important part in the development process. There are many reasons to say this:

  • Getting other developers feedback minimizes the risk of adding regressions to the code base and ensures the quality of the code being merged.
  • It helps to build community. Everyone appreciate when their code is reviewed, and of course they will review your code when they have their chance.
  • It also helps developers to improve, since they are always learning from other's points of view.
  • Its a great practice to get familiar with the code.


Everyone is encouraged to review code! It's not necessary that you know every detail of the code base, you only have to understand what the code related to the fix does.

Step by step

  • Go to review.openstack.org and filter by Open Zaqar fixes.
  • Select a fix from the list to review.
  • Download the patch to your local repository and test it.


git review -d [review-id]

The review-id is located the number in the URL (check the screenshot for more details).


Example

git review -d 92979


Download the patch using the Review ID in the address bar.

This will create a branch with the author name and will allow you to test the patch in your local environment.

  • Inspect the code. Is important that you consider all the good programming practices you know here. Take a look to the checklist for some ideas.
  • If during the inspection you see a specific line you would like to bring up to discussion in the final review, leave your inline comment. This will make the review process easier. As you compose your comments, please keep in mind these guidelines.
  • Hit the "Review" button in the web UI.
  • Time to score!
    • When to score with -1? If you think that there are things that should be fixed, then voting -1 is ok. We have to be careful with this since we don't want to stall the cycle just because a few nits, so downvoting also depends on the current stage of the development cycle and the severity of the flaw you see.
    • When to score with 0? If you are the author of the fix and you want respond to the reviewers comments, or if you are a reviewer and you want to point out some reminder for future developing (e.g. the deadline is the next day and the fix needs to be merged, but you want something to be improved).
    • When to score with +1? If the fix works and you think that the code looks good, upvoting is your choice.
  • Remember to leave any comment you think is important in the comment form, and when you are done just click "Publish Comments".


For more details on how to do a review, check out the review section in the Developer's Guide.

Checklist

  • No idea on how to start? Take a moment to check different reviews for patches in review.openstack.org. Don't forget about inline comments! Also, try to select an easy patch for your first review. That will help you to gain some confidence.
  • If you already submitted a patch, take a moment to think about the reviews you received. Which were the most useful comments? Consider the feedback that helped you the most in the past and try to do the same for the fix you are reviewing. Helpful review comments will be appreciated by the developer that is submitting the fix.
  • Code location feedback. Do you consider that some code should be better located in another place within the file, or maybe in another file? If so, suggest it the review comment and score with -1 if you think that it's that important.
  • Code style feedback. Do you think that the code structure could be improved? Keep the DRY, YAGNI and KISS principles in mind.
  • Grammar/orthography feedback. Many of our contributors are not native English speakers, so its common to find some of this errors. Git Commit messages are as important as code, so you should review it too. Check GitCommitMessages for more information on how a Git Commit message should be written.