Tech Blog: April 2013

This post describes how to set up a custom Code Review policy in TFS 2012. It follows on from an excellent article by Jacob Maki which only required a few extra lines of code to get it working. I then extended it by adding an additional boolean variable parameter so that it could be enabled or disabled on the build process template - this would be useful if you wanted to configure code review policy on a per-branch basis.

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.

100

101

102

103

104

105

106

107

108

109

110

111

112

113

114

115

116

117

118

119

120

121

122

123

124

125

126

127

128

129

130

131

132

133

134

135

136

137

138

139

140

141

142

143

144

145

146

147

148

149

150

151

152

153

154

155

156

157

158

159

160

161

162

163

164

165

166

167

168

169

170

171

172

173

174

175

176

177

178

179

180

181

182

183

184

185

186

187

188

189

190

191

192

193

194

195

196

197

198

199

200

201

202

203

204

205

206

207

208

209

210

211

212

213

214

215

216

217

218

219

220

221

222

223

224

225

226

227

228

229

230

231

232

233

234

235

236

237

238

239

240

241

242

243

244

245

246

247

248

249

250

251

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
    {
        ///<summary>
        /// Gets or sets the build detail
        ///</summary>
        [RequiredArgument]
        [Browsable(true)]
        public InArgument<ibuilddetail> BuildDetail { get; set; }
 
        ///<summary>
        /// Gets or sets the team project collection.  Use Microsoft's GetTeamProjectCollection activity.
        ///</summary>
        [RequiredArgument]
        [Browsable(true)]
        public InArgument<tfsteamprojectcollection> TeamProjectCollection { get; set; }
 
        ///<summary>
        /// Gets or sets the changesets to check for code reviews
        ///</summary>
        [RequiredArgument]
        [Browsable(true)]
        public InArgument<ilist hangeset="">> Changesets { get; set; }
 
        ///<summary>
        /// Gets or sets the comma separated list of files types to check (for example, ".sql,.cs").  An
        /// empty string means all files require review.
        ///</summary>
        [Browsable(true)]
        public InArgument<string> CommaSeparatedFileTypes { get; set; }
 
        ///<summary>
        /// Gets or sets the comma separated list of files types to check (for example, ".sql,.cs").  An
        /// empty string means all files require review.
        ///</summary>
        [Browsable(true)]
        public InArgument<bool> EnableCodeReview { get; set; }
 
        ///<summary>
        /// Gets or sets a value indicating whether the build will fail if there is non-reviewed code
        /// matching the specified file types.
        ///</summary>
        [Browsable(true)]
        [RequiredArgument]
        public InArgument<bool> FailBuildOnNonReviewedCode { get; set; }
 
 
        /// <summary>
        /// Performs the execution of the activity
        /// </summary>
        /// <param name="context">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<changeset> 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<string> filesTypesRequiringReview = ParseFileTypesRequiringReview(context.GetValue(CommaSeparatedFileTypes));
             
            WorkItemStore workItemStore = teamProjectCollection.GetService<workitemstore>();
            if (workItemStore == null) throw new NullReferenceException("Cannot access the WorkItemStore");
 
            WriteBuildMessage(context, "Checking whether any files in the changesets require a review");
            IEnumerable<change> 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"));
            }
        }
 
        /// <summary>
        /// Determines if the build has any changesets that have not been reviewed
        /// </summary>
        /// <param name="workItemStore">Work item store
        /// <param name="changesets">Changesets
        /// <param name="filesTypesRequiringReview">File types requiring review (empty collection means all)
        /// <returns>Changes without a passing review</returns>
        private IEnumerable<change> DoesBuildHaveNonReviewedCode(CodeActivityContext context, WorkItemStore workItemStore, IList<changeset> changesets, ICollection<string> 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;
                        }
                    }
                }
            }
        }
 
        /// <summary>
        /// Determines if the change requires review
        /// </summary>
        /// <param name="change">Change
        /// <param name="fileTypesRequiringReview">File types requiring review
        /// <returns>True if a passing review is required, else false</returns>
        private static bool DoesChangeRequireReview(Change change, ICollection<string> fileTypesRequiringReview)
        {
            bool matchingFileToCheck = fileTypesRequiringReview.Count == 0;
 
            foreach (string fileType in fileTypesRequiringReview)
            {
                if (change.Item.ServerItem.EndsWith(fileType, StringComparison.CurrentCultureIgnoreCase))
                {
                    matchingFileToCheck = true;
                }
            }
 
            return matchingFileToCheck;
        }
 
        /// <summary>
        /// Determines if the changeset has a passing code review associated with it
        /// </summary>
        /// <param name="workItemStore">Work item store
        /// <param name="changeset">Changeset
        /// <returns>True if the changeset has a passing review, else false</returns>
        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;
        }
 
        /// <summary>
        /// Takes the appropriate actions when a changeset does not have a passing review
        /// </summary>
        /// <param name="context">Context
        /// <param name="treatAsError">True if the message should be treated as an error, else it will be a warning
        /// <param name="change">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<string> 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.";
    }
}</string></string></string></changeset></change></change></workitemstore></string></changeset></bool></bool></string></ilist></tfsteamprojectcollection></ibuilddetail>

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

Tech Blog

Saturday, 27 April 2013

Set security on Freenas shares

Thursday, 25 April 2013

Code Review policy for TFS 2012 - Custom Activity in an Agile Template

Blog Archive