<html>
<head>
    <base href="https://wiki.asterisk.org/wiki">
            <link rel="stylesheet" href="/wiki/s/2036/1/7/_/styles/combined.css?spaceKey=TOP&amp;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&amp;showCommentArea=true#addcomment">Add Comment</a>
            </div>
</div>
</div>
</div>
</div>
</body>
</html>