So, what are we trying to achieve?
The goal here is to force developers to undergo Code Review before checking in code. TFS doesn't do this out of the box, but there are two ways of doing this:
1. Create a custom check-in policy. This is nice because it tells the user that they must perform a code-review before checking in - it stops the problem at source. However it requires a custom check-in policy which then must be distributed to all developers and it cannot be configured on a per-branch basis (unless you start coding in branch names in the code). Here is another example.2. Alter the build process template so it checks the changesets for code that should have been reviewed. This can be done without distributing code to the developers and can be altered on a branch-by-branch basis, but it will highlight the problem after the code has been checked in and not before.
Option 1 can be simplified using TFS PowerTools for distributing the check-in policy, but for my needs, option 2 worked best. We wanted to enforce code reviews for the Main and Sprint branches, but not for Task branches.
Step one is to write a Custom Activity.
using Microsoft.TeamFoundation.Build.Client;
using Microsoft.TeamFoundation.Build.Workflow.Activities;
using Microsoft.TeamFoundation.Client;
using Microsoft.TeamFoundation.VersionControl.Client;
using Microsoft.TeamFoundation.WorkItemTracking.Client;
using System;
using System.Activities;
using System.Collections.Generic;
using System.ComponentModel;
using System.Globalization;
using System.Linq;
using System.Text;
namespace CustomBuildActivities
{
[BuildActivity(HostEnvironmentOption.Agent)]
public sealed class CodeReviewPolicy : CodeActivity
{
///
/// Gets or sets the build detail
///
[RequiredArgument]
[Browsable(true)]
public InArgument BuildDetail { get; set; }
///
/// Gets or sets the team project collection. Use Microsoft's GetTeamProjectCollection activity.
///
[RequiredArgument]
[Browsable(true)]
public InArgument TeamProjectCollection { get; set; }
///
/// Gets or sets the changesets to check for code reviews
///
[RequiredArgument]
[Browsable(true)]
public InArgument> Changesets { get; set; }
///
/// Gets or sets the comma separated list of files types to check (for example, ".sql,.cs"). An
/// empty string means all files require review.
///
[Browsable(true)]
public InArgument CommaSeparatedFileTypes { get; set; }
///
/// Gets or sets the comma separated list of files types to check (for example, ".sql,.cs"). An
/// empty string means all files require review.
///
[Browsable(true)]
public InArgument EnableCodeReview { get; set; }
///
/// Gets or sets a value indicating whether the build will fail if there is non-reviewed code
/// matching the specified file types.
///
[Browsable(true)]
[RequiredArgument]
public InArgument FailBuildOnNonReviewedCode { get; set; }
///
/// Performs the execution of the activity
///
/// The execution context under which the activity executes
protected override void Execute(CodeActivityContext context)
{
#region Validate arguments and parameters
if (context == null) { throw new ArgumentNullException("context"); }
TfsTeamProjectCollection teamProjectCollection = context.GetValue(TeamProjectCollection);
if (teamProjectCollection == null) { throw new ArgumentException("Team Project Collection cannot be null."); }
IBuildDetail buildDetail = context.GetValue(BuildDetail);
if (buildDetail == null) { throw new ArgumentException("Build Detail cannot be null."); }
IList changesets = context.GetValue(Changesets);
if (changesets == null) { throw new ArgumentException("Changesets cannot be null."); }
#endregion
bool enableCodeReview = context.GetValue(EnableCodeReview);
bool failBuildOnNonReviewedCode = context.GetValue(FailBuildOnNonReviewedCode);
if (!enableCodeReview)
{
WriteBuildMessage(context, "Code review policy is disabled");
return;
}
ICollection filesTypesRequiringReview = ParseFileTypesRequiringReview(context.GetValue(CommaSeparatedFileTypes));
WorkItemStore workItemStore = teamProjectCollection.GetService();
if (workItemStore == null) throw new NullReferenceException("Cannot access the WorkItemStore");
WriteBuildMessage(context, "Checking whether any files in the changesets require a review");
IEnumerable nonReviewedChanges = DoesBuildHaveNonReviewedCode(context, workItemStore, changesets, filesTypesRequiringReview);
if (nonReviewedChanges == null) throw new NullReferenceException("NonReviewedChanges is null");
WriteBuildMessage(context, string.Format("There are {0} non-reviewed changes", nonReviewedChanges.Count()));
bool anyNonReviewedChangesets = false;
foreach (Change nonReviewedChange in nonReviewedChanges)
{
anyNonReviewedChangesets = true;
HandleChangeWithoutPassingReview(context, failBuildOnNonReviewedCode, nonReviewedChange);
}
if (anyNonReviewedChangesets && failBuildOnNonReviewedCode)
{
buildDetail.Status = BuildStatus.Failed;
}
else
{
WriteBuildMessage(context, string.Format("Code review check passed"));
}
}
///
/// Determines if the build has any changesets that have not been reviewed
///
/// Work item store
/// Changesets
/// File types requiring review (empty collection means all)
/// Changes without a passing review
private IEnumerable DoesBuildHaveNonReviewedCode(CodeActivityContext context, WorkItemStore workItemStore, IList changesets, ICollection filesTypesRequiringReview)
{
if (changesets == null) throw new ArgumentNullException("changesets", "No Changesets specified");
if (changesets == null || changesets.Count == 0) WriteBuildMessage(context, string.Format("There are {0} changesets associated with this build", changesets.Count));
foreach (Changeset changeset in changesets)
{
// The changeset object passed in may not contain the associated changes (depending on how this is executed - workflow versus unit test)
Change[] changes = changeset.Changes.Length > 0 ? changeset.Changes : changeset.VersionControlServer.GetChangesForChangeset(changeset.ChangesetId, false, int.MaxValue, null);
if (changes == null || changes.Length == 0)
{
WriteBuildMessage(context, string.Format("Changeset {0} has 0 associated changes", changeset.ChangesetId));
}
foreach (Change change in changes)
{
bool matchingFileToCheck = DoesChangeRequireReview(change, filesTypesRequiringReview);
if (matchingFileToCheck)
{
bool hasPassingCodeReview = DoesChangesetHavePassingReview(workItemStore, changeset);
if (!hasPassingCodeReview)
{
yield return change;
}
}
}
}
}
///
/// Determines if the change requires review
///
/// Change
/// File types requiring review
/// True if a passing review is required, else false
private static bool DoesChangeRequireReview(Change change, ICollection fileTypesRequiringReview)
{
bool matchingFileToCheck = fileTypesRequiringReview.Count == 0;
foreach (string fileType in fileTypesRequiringReview)
{
if (change.Item.ServerItem.EndsWith(fileType, StringComparison.CurrentCultureIgnoreCase))
{
matchingFileToCheck = true;
}
}
return matchingFileToCheck;
}
///
/// Determines if the changeset has a passing code review associated with it
///
/// Work item store
/// Changeset
/// True if the changeset has a passing review, else false
private static bool DoesChangesetHavePassingReview(WorkItemStore workItemStore, Changeset changeset)
{
Query query = new Query(workItemStore,
string.Format(CultureInfo.InvariantCulture,
"SELECT [Id] FROM WorkItemLinks WHERE "
+ "[Source].[System.WorkItemType] = 'Code Review Request' "
+ "AND [Source].[Microsoft.VSTS.CodeReview.Context] = '{0}' "
+ "AND [System.Links.LinkType] = 'Child' "
+ "AND [Target].[System.WorkItemType] = 'Code Review Response' "
+ "AND ([Target].[Microsoft.VSTS.CodeReview.ClosedStatusCode] = 1 OR [Target].[Microsoft.VSTS.CodeReview.ClosedStatusCode] = 2) " // 1 = Looks Good, 2 = With Comments
+ "mode(MustContain)",
changeset.ChangesetId)
);
return query.RunCountQuery() > 0;
}
///
/// Takes the appropriate actions when a changeset does not have a passing review
///
/// Context
/// True if the message should be treated as an error, else it will be a warning
/// Change
private static void HandleChangeWithoutPassingReview(CodeActivityContext context, bool treatAsError, Change change)
{
if (treatAsError)
{
WriteBuildError(context, string.Format(CultureInfo.CurrentCulture, UnreviewedFileMessageTemplate, change.Item.ServerItem));
}
else
{
WriteBuildWarning(context, string.Format(CultureInfo.CurrentCulture, UnreviewedFileMessageTemplate, change.Item.ServerItem));
}
}
private static void WriteBuildError(CodeActivityContext context, string message)
{
if (context == null) throw new ArgumentNullException("context");
context.TrackBuildError(message);
}
private static void WriteBuildWarning(CodeActivityContext context, string message)
{
if (context == null) throw new ArgumentNullException("context");
context.TrackBuildWarning(message);
}
private static void WriteBuildMessage(CodeActivityContext context, string message)
{
if (context == null) throw new ArgumentNullException("context");
context.TrackBuildMessage(message);
}
private static ICollection ParseFileTypesRequiringReview(string commaSeparatedFileTypes)
{
if (string.IsNullOrEmpty(commaSeparatedFileTypes))
{
throw new ArgumentNullException("commaSeparatedFileTypes", "Please specify the file types to be checked for code review");
}
return commaSeparatedFileTypes.Split(new[] { ',' });
}
public static readonly string UnreviewedFileMessageTemplate = "{0} has not passed code review.";
}
}
Note the referenced assemblies:
Build this and then check it in to the CustomAssemblies path defined for your build controller.
At this point you may wish to add other assemblies, such as those from the TFS Community Build Extensions.
Mine are here:
Now alter the build process template. I am using the MS Scrum 2.1 process template and (a copy of) the corresponding default build process template.
I built a simple project, added a reference to my new Custom Activity, other references and my build process template:
Add your new activity onto the Toolbox using the Choose Items... context menu.
Find the If AssociateChangesetsAndWorkItems activity and then the child Associate Changesets and Work Items for non-Shelveset Builds. Add a GetTeamProjectCollection activity from the toolbox and then add the CodeReviewPolicy activity.
Edit the properties of the CodeReviewPolicy. You can consume variables that have been set up earlier. Some variables, such as the output of GetTeamProjectCollection, require references to additional assemblies, but these are shown in the screenshot for the Build
No comments:
Post a Comment