<html>
<head>
<base href="https://wiki.asterisk.org/wiki">
<link rel="stylesheet" href="/wiki/s/2036/1/7/_/styles/combined.css?spaceKey=TOP&forWysiwyg=true" type="text/css">
</head>
<body style="background: white;" bgcolor="white" class="email-body">
<div id="pageContent">
<div id="notificationFormat">
<div class="wiki-content">
<div class="email">
<h2><a href="https://wiki.asterisk.org/wiki/display/TOP/Code+Reviews">Code Reviews</a></h2>
<h4>Page <b>edited</b> by <a href="https://wiki.asterisk.org/wiki/display/~dlee">David M. Lee</a>
</h4>
<br/>
<h4>Changes (1)</h4>
<div id="page-diffs">
<table class="diff" cellpadding="0" cellspacing="0">
<tr><td class="diff-snipped" >...<br></td></tr>
<tr><td class="diff-unchanged" > <br># Developer writes some code. <br></td></tr>
<tr><td class="diff-added-lines" style="background-color: #dfd;">#* With sufficient [automated tests|Asterisk SCF Testing], of course. <br></td></tr>
<tr><td class="diff-unchanged" ># Developer pushes code to an integration repo. <br># _Repeat until code is ready for review._ <br></td></tr>
<tr><td class="diff-snipped" >...<br></td></tr>
</table>
</div> <h4>Full Content</h4>
<div class="notificationGreySide">
<p>We use <a href="https://code.asterisk.org/code/cru/browse/CR-ASTSCF" class="external-link" rel="nofollow">Crucible</a> for managing code reviews for Asterisk SCF. I recommend Asterisk SCF contributors quickly peruse the <a href="http://confluence.atlassian.com/display/CRUCIBLE/Crucible+User%27s+Guide" class="external-link" rel="nofollow">Crucible User's Guide</a> for details on working with the tool.</p>
<p>We use a slightly modified <a href="http://confluence.atlassian.com/display/CRUCIBLE/Agile+Permissions+Schemes+in+Crucible" class="external-link" rel="nofollow">Agile permission scheme</a> for our reviews. The modifications are:</p>
<ul>
<li>The Moderator role is disabled.</li>
<li>Only Asterisk SCF contributors may create or comment on reviews.
<ul>
<li>To become a contributor, please see the <a href="/wiki/display/TOP/Community" title="Community">Community</a> page for more information.</li>
</ul>
</li>
</ul>
<h1><a name="CodeReviews-Workflow"></a>Workflow</h1>
<p>By design, the workflow for Asterisk SCF code reviews is very open and non-restrictive. Any contributor can create a review from an uploaded patch, a commit changeset, a particular revision of a file in any of the release or integration repos.</p>
<p>However, most code reviews will follow a relatively simple process.</p>
<ol>
<li>Developer writes some code.
<ul>
<li>With sufficient <a href="/wiki/display/TOP/Asterisk+SCF+Testing" title="Asterisk SCF Testing">automated tests</a>, of course.</li>
</ul>
</li>
<li>Developer pushes code to an integration repo.</li>
<li><em>Repeat until code is ready for review.</em></li>
<li>Developer goes to <a href="http://code.asterisk.org" class="external-link" rel="nofollow">http://code.asterisk.org</a> and creates a code review for their changes.
<ul>
<li>When selecting reviewers, be courteous take a look at the <a href="https://code.asterisk.org/code/plugins/servlet/review-blockers/" class="external-link" rel="nofollow">review blockers report</a> to see if anyone's overloaded with reviews.</li>
</ul>
</li>
</ol>
<p>Then:</p>
<ol>
<li>Reviewers comment on the code for review.</li>
<li>Developer responds to comment.</li>
<li>Developer updates code in response to the review.
<ul>
<li>Or the developer can create a Jira issue for later action.</li>
</ul>
</li>
<li>When a reviewer runs out of things to say, the reviewer completes their review.
<ul>
<li>Crucible will automagically un-complete the review if anyone adds or updates the code in the review.</li>
</ul>
</li>
<li><em>Repeat until the code is awesome</em>.</li>
</ol>
<p>Finally:</p>
<ol>
<li>Developer summarizes the review.</li>
<li>Developer merges into the release master branch.
<ul>
<li>If it's reasonable to do this with a rebase, do so to avoid excessive merge commits. If the rebase goes wrong, then you can use <a href="http://www.kernel.org/pub/software/scm/git/docs/git-reflog.html" class="external-link" rel="nofollow">git reflog</a> and <a href="http://www.kernel.org/pub/software/scm/git/docs/git-reset.html" class="external-link" rel="nofollow">git reset --hard</a> to get back to your pre-rebase goodness.</li>
</ul>
</li>
<li>Developer deletes the topic branch, both locally and remotely</li>
</ol>
<p>The cheat sheet for the final bit of git magic is as follows:</p>
<div class="code panel" style="border-width: 1px;"><div class="codeContent panelContent">
<script type="syntaxhighlighter" class="toolbar: false; theme: Confluence; brush: bash; gutter: false"><![CDATA[
# get latest changes from the server
git fetch origin
# checkout the topic branch
git checkout topic-branch
# rebase to master
git rebase origin/master
# checkout master
git checkout master
# merge changes from the topic branch. should be just a fast forward
git merge topic-branch
# push the changes
git push
# delete the local topic branch
git branch -d topic-branch
# delete the remote topic branch
git push integration :topic-branch
]]></script>
</div></div>
</div>
<div id="commentsSection" class="wiki-content pageSection">
<div style="float: right;">
<a href="https://wiki.asterisk.org/wiki/users/viewnotifications.action" class="grey">Change Notification Preferences</a>
</div>
<a href="https://wiki.asterisk.org/wiki/display/TOP/Code+Reviews">View Online</a>
|
<a href="https://wiki.asterisk.org/wiki/pages/diffpagesbyversion.action?pageId=10650101&revisedVersion=2&originalVersion=1">View Changes</a>
|
<a href="https://wiki.asterisk.org/wiki/display/TOP/Code+Reviews?showComments=true&showCommentArea=true#addcomment">Add Comment</a>
</div>
</div>
</div>
</div>
</div>
</body>
</html>