From fff31e11c5208854164adb5e2532b822fbca7574 Mon Sep 17 00:00:00 2001 From: Luke Tomlinson Date: Tue, 13 Jul 2021 11:38:16 -0400 Subject: [PATCH] Add notice annotation level and support more annotation fields (#1175) * Add Notice Command * Add Feature Flag For Enhanced Annotations * Fix tests * Add validation for columns and lines * Fix order to match service * Remove console.write * Make Validation Better * Cleanup * Handle empty/whitespace strings * Add more validation for line/column ranges * Make Validation Debug, Not Throw * Change casing to :camel: from :snake: * Give notice a well known tag * Cleanup * Sanitize invalid commands rather than fail --- src/Runner.Common/ExtensionManager.cs | 1 + src/Runner.Common/JobServerQueue.cs | 5 ++ src/Runner.Worker/ActionCommandManager.cs | 84 +++++++++++++++++++- src/Runner.Worker/ExecutionContext.cs | 20 +++++ src/Runner.Worker/Handlers/OutputManager.cs | 4 + src/Sdk/DTWebApi/WebApi/IssueType.cs | 5 +- src/Sdk/DTWebApi/WebApi/TimelineRecord.cs | 8 ++ src/Test/L0/Worker/ActionCommandManagerL0.cs | 83 +++++++++++++++++++ 8 files changed, 208 insertions(+), 2 deletions(-) diff --git a/src/Runner.Common/ExtensionManager.cs b/src/Runner.Common/ExtensionManager.cs index 09a094c1c..a432d6d39 100644 --- a/src/Runner.Common/ExtensionManager.cs +++ b/src/Runner.Common/ExtensionManager.cs @@ -51,6 +51,7 @@ namespace GitHub.Runner.Common Add(extensions, "GitHub.Runner.Worker.RemoveMatcherCommandExtension, Runner.Worker"); Add(extensions, "GitHub.Runner.Worker.WarningCommandExtension, Runner.Worker"); Add(extensions, "GitHub.Runner.Worker.ErrorCommandExtension, Runner.Worker"); + Add(extensions, "GitHub.Runner.Worker.NoticeCommandExtension, Runner.Worker"); Add(extensions, "GitHub.Runner.Worker.DebugCommandExtension, Runner.Worker"); Add(extensions, "GitHub.Runner.Worker.GroupCommandExtension, Runner.Worker"); Add(extensions, "GitHub.Runner.Worker.EndGroupCommandExtension, Runner.Worker"); diff --git a/src/Runner.Common/JobServerQueue.cs b/src/Runner.Common/JobServerQueue.cs index 5cabca2a9..785637e57 100644 --- a/src/Runner.Common/JobServerQueue.cs +++ b/src/Runner.Common/JobServerQueue.cs @@ -544,6 +544,11 @@ namespace GitHub.Runner.Common timelineRecord.WarningCount = rec.WarningCount; } + if (rec.NoticeCount != null && rec.NoticeCount > 0) + { + timelineRecord.NoticeCount = rec.NoticeCount; + } + if (rec.Issues.Count > 0) { timelineRecord.Issues.Clear(); diff --git a/src/Runner.Worker/ActionCommandManager.cs b/src/Runner.Worker/ActionCommandManager.cs index f10e159f3..75588aca3 100644 --- a/src/Runner.Worker/ActionCommandManager.cs +++ b/src/Runner.Worker/ActionCommandManager.cs @@ -75,6 +75,12 @@ namespace GitHub.Runner.Worker return false; } + if (!ActionCommandManager.EnhancedAnnotationsEnabled(context) && actionCommand.Command == "notice") + { + context.Debug($"Enhanced Annotations not enabled on the server: 'notice' command will not be processed."); + return false; + } + // Serialize order lock (_commandSerializeLock) { @@ -141,6 +147,10 @@ namespace GitHub.Runner.Worker return true; } + + internal static bool EnhancedAnnotationsEnabled(IExecutionContext context) { + return context.Global.Variables.GetBoolean("DistributedTask.EnhancedAnnotations") ?? false; + } } public interface IActionCommandExtension : IExtension @@ -498,6 +508,13 @@ namespace GitHub.Runner.Worker public override string Command => "error"; } + public sealed class NoticeCommandExtension : IssueCommandExtension + { + public override IssueType Type => IssueType.Notice; + + public override string Command => "notice"; + } + public abstract class IssueCommandExtension : RunnerService, IActionCommandExtension { public abstract IssueType Type { get; } @@ -512,6 +529,11 @@ namespace GitHub.Runner.Worker command.Properties.TryGetValue(IssueCommandProperties.Line, out string line); command.Properties.TryGetValue(IssueCommandProperties.Column, out string column); + if (!ActionCommandManager.EnhancedAnnotationsEnabled(context)) + { + context.Debug("Enhanced Annotations not enabled on the server. The 'title', 'end_line', and 'end_column' fields are unsupported."); + } + Issue issue = new Issue() { Category = "General", @@ -563,13 +585,73 @@ namespace GitHub.Runner.Worker context.AddIssue(issue); } + public static void ValidateLinesAndColumns(ActionCommand command, IExecutionContext context) + { + command.Properties.TryGetValue(IssueCommandProperties.Line, out string line); + command.Properties.TryGetValue(IssueCommandProperties.EndLine, out string endLine); + command.Properties.TryGetValue(IssueCommandProperties.Column, out string column); + command.Properties.TryGetValue(IssueCommandProperties.EndColumn, out string endColumn); + + var hasStartLine = int.TryParse(line, out int lineNumber); + var hasEndLine = int.TryParse(endLine, out int endLineNumber); + var hasStartColumn = int.TryParse(column, out int columnNumber); + var hasEndColumn = int.TryParse(endColumn, out int endColumnNumber); + var hasColumn = hasStartColumn || hasEndColumn; + + if (hasEndLine && !hasStartLine) + { + context.Debug($"Invalid {command.Command} command value. '{IssueCommandProperties.EndLine}' can only be set if '{IssueCommandProperties.Line}' is provided"); + command.Properties[IssueCommandProperties.Line] = endLine; + hasStartLine = true; + line = endLine; + } + + if (hasEndColumn && !hasStartColumn) + { + context.Debug($"Invalid {command.Command} command value. '{IssueCommandProperties.EndColumn}' can only be set if '{IssueCommandProperties.Column}' is provided"); + command.Properties[IssueCommandProperties.Column] = endColumn; + hasStartColumn = true; + column = endColumn; + } + + if (!hasStartLine && hasColumn) + { + context.Debug($"Invalid {command.Command} command value. '{IssueCommandProperties.Column}' and '{IssueCommandProperties.EndColumn}' can only be set if '{IssueCommandProperties.Line}' value is provided."); + command.Properties.Remove(IssueCommandProperties.Column); + command.Properties.Remove(IssueCommandProperties.EndColumn); + } + + if (hasEndLine && line != endLine && hasColumn) + { + context.Debug($"Invalid {command.Command} command value. '{IssueCommandProperties.Column}' and '{IssueCommandProperties.EndColumn}' cannot be set if '{IssueCommandProperties.Line}' and '{IssueCommandProperties.EndLine}' are different values."); + command.Properties.Remove(IssueCommandProperties.Column); + command.Properties.Remove(IssueCommandProperties.EndColumn); + } + + if (hasStartLine && hasEndLine && endLineNumber < lineNumber) + { + context.Debug($"Invalid {command.Command} command value. '{IssueCommandProperties.EndLine}' cannot be less than '{IssueCommandProperties.Line}'."); + command.Properties.Remove(IssueCommandProperties.Line); + command.Properties.Remove(IssueCommandProperties.EndLine); + } + + if (hasStartColumn && hasEndColumn && endColumnNumber < columnNumber) + { + context.Debug($"Invalid {command.Command} command value. '{IssueCommandProperties.EndColumn}' cannot be less than '{IssueCommandProperties.Column}'."); + command.Properties.Remove(IssueCommandProperties.Column); + command.Properties.Remove(IssueCommandProperties.EndColumn); + } + } + private static class IssueCommandProperties { public const String File = "file"; public const String Line = "line"; + public const String EndLine = "endLine"; public const String Column = "col"; + public const String EndColumn = "endColumn"; + public const String Title = "title"; } - } public sealed class GroupCommandExtension : GroupingCommandExtension diff --git a/src/Runner.Worker/ExecutionContext.cs b/src/Runner.Worker/ExecutionContext.cs index f0890b8ee..748e6b338 100644 --- a/src/Runner.Worker/ExecutionContext.cs +++ b/src/Runner.Worker/ExecutionContext.cs @@ -516,6 +516,24 @@ namespace GitHub.Runner.Worker } _record.WarningCount++; + } + else if (issue.Type == IssueType.Notice) + { + + // tracking line number for each issue in log file + // log UI use this to navigate from issue to log + if (!string.IsNullOrEmpty(logMessage)) + { + long logLineNumber = Write(WellKnownTags.Notice, logMessage); + issue.Data["logFileLineNumber"] = logLineNumber.ToString(); + } + + if (_record.NoticeCount < _maxIssueCount) + { + _record.Issues.Add(issue); + } + + _record.NoticeCount++; } _jobServerQueue.QueueTimelineRecordUpdate(_mainTimelineId, _record); @@ -841,6 +859,7 @@ namespace GitHub.Runner.Worker _record.State = TimelineRecordState.Pending; _record.ErrorCount = 0; _record.WarningCount = 0; + _record.NoticeCount = 0; if (parentTimelineRecordId != null && parentTimelineRecordId.Value != Guid.Empty) { @@ -1012,6 +1031,7 @@ namespace GitHub.Runner.Worker public static readonly string Command = "##[command]"; public static readonly string Error = "##[error]"; public static readonly string Warning = "##[warning]"; + public static readonly string Notice = "##[notice]"; public static readonly string Debug = "##[debug]"; } } diff --git a/src/Runner.Worker/Handlers/OutputManager.cs b/src/Runner.Worker/Handlers/OutputManager.cs index 855e6bd02..03bbe789f 100644 --- a/src/Runner.Worker/Handlers/OutputManager.cs +++ b/src/Runner.Worker/Handlers/OutputManager.cs @@ -210,6 +210,10 @@ namespace GitHub.Runner.Worker.Handlers { issueType = DTWebApi.IssueType.Warning; } + else if (string.Equals(match.Severity, "notice", StringComparison.OrdinalIgnoreCase)) + { + issueType = DTWebApi.IssueType.Notice; + } else { _executionContext.Debug($"Skipped logging an issue for the matched line because the severity '{match.Severity}' is not supported."); diff --git a/src/Sdk/DTWebApi/WebApi/IssueType.cs b/src/Sdk/DTWebApi/WebApi/IssueType.cs index 8b8e52d1e..603d0b59c 100644 --- a/src/Sdk/DTWebApi/WebApi/IssueType.cs +++ b/src/Sdk/DTWebApi/WebApi/IssueType.cs @@ -9,6 +9,9 @@ namespace GitHub.DistributedTask.WebApi Error = 1, [EnumMember] - Warning = 2 + Warning = 2, + + [EnumMember] + Notice = 3 } } diff --git a/src/Sdk/DTWebApi/WebApi/TimelineRecord.cs b/src/Sdk/DTWebApi/WebApi/TimelineRecord.cs index a14bded63..51317d171 100644 --- a/src/Sdk/DTWebApi/WebApi/TimelineRecord.cs +++ b/src/Sdk/DTWebApi/WebApi/TimelineRecord.cs @@ -38,6 +38,7 @@ namespace GitHub.DistributedTask.WebApi this.RefName = recordToBeCloned.RefName; this.ErrorCount = recordToBeCloned.ErrorCount; this.WarningCount = recordToBeCloned.WarningCount; + this.NoticeCount = recordToBeCloned.NoticeCount; this.AgentPlatform = recordToBeCloned.AgentPlatform; if (recordToBeCloned.Log != null) @@ -222,6 +223,13 @@ namespace GitHub.DistributedTask.WebApi set; } + [DataMember(Order = 55)] + public Int32? NoticeCount + { + get; + set; + } + public List Issues { get diff --git a/src/Test/L0/Worker/ActionCommandManagerL0.cs b/src/Test/L0/Worker/ActionCommandManagerL0.cs index 3bea6eeb1..f9080dbfc 100644 --- a/src/Test/L0/Worker/ActionCommandManagerL0.cs +++ b/src/Test/L0/Worker/ActionCommandManagerL0.cs @@ -188,6 +188,84 @@ namespace GitHub.Runner.Common.Tests.Worker } } + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void IssueCommandInvalidColumns() + { + using (TestHostContext hc = CreateTestContext()) + { + _ec.Setup(x => x.Write(It.IsAny(), It.IsAny())) + .Returns((string tag, string line) => + { + hc.GetTrace().Info($"{tag} {line}"); + return 1; + }); + + var registeredCommands = new HashSet(new string[1]{ "warning" }); + ActionCommand command; + + // Columns when lines are different + ActionCommand.TryParseV2("::warning line=1,endLine=2,col=1,endColumn=2::this is a warning", registeredCommands, out command); + Assert.Equal("1", command.Properties["col"]); + IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object); + Assert.False(command.Properties.ContainsKey("col")); + + // No lines with columns + ActionCommand.TryParseV2("::warning col=1,endColumn=2::this is a warning", registeredCommands, out command); + Assert.Equal("1", command.Properties["col"]); + Assert.Equal("2", command.Properties["endColumn"]); + IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object); + Assert.False(command.Properties.ContainsKey("col")); + Assert.False(command.Properties.ContainsKey("endColumn")); + + // No line with endLine + ActionCommand.TryParseV2("::warning endLine=1::this is a warning", registeredCommands, out command); + Assert.Equal("1", command.Properties["endLine"]); + IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object); + Assert.Equal(command.Properties["endLine"], command.Properties["line"]); + + // No column with endColumn + ActionCommand.TryParseV2("::warning line=1,endColumn=2::this is a warning", registeredCommands, out command); + Assert.Equal("2", command.Properties["endColumn"]); + IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object); + Assert.Equal(command.Properties["endColumn"], command.Properties["col"]); + + // Empty Strings + ActionCommand.TryParseV2("::warning line=,endLine=3::this is a warning", registeredCommands, out command); + IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object); + Assert.Equal(command.Properties["line"], command.Properties["endLine"]); + + // Nonsensical line values + ActionCommand.TryParseV2("::warning line=4,endLine=3::this is a warning", registeredCommands, out command); + IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object); + Assert.False(command.Properties.ContainsKey("line")); + Assert.False(command.Properties.ContainsKey("endLine")); + + /// Nonsensical column values + ActionCommand.TryParseV2("::warning line=1,endLine=1,col=3,endColumn=2::this is a warning", registeredCommands, out command); + IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object); + Assert.False(command.Properties.ContainsKey("col")); + Assert.False(command.Properties.ContainsKey("endColumn")); + + // Valid + ActionCommand.TryParseV2("::warning line=1,endLine=1,col=1,endColumn=2::this is a warning", registeredCommands, out command); + IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object); + Assert.Equal("1", command.Properties["line"]); + Assert.Equal("1", command.Properties["endLine"]); + Assert.Equal("1", command.Properties["col"]); + Assert.Equal("2", command.Properties["endColumn"]); + + // Backwards compatibility + ActionCommand.TryParseV2("::warning line=1,col=1,file=test.txt::this is a warning", registeredCommands, out command); + IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object); + Assert.Equal("1", command.Properties["line"]); + Assert.False(command.Properties.ContainsKey("endLine")); + Assert.Equal("1", command.Properties["col"]); + Assert.False(command.Properties.ContainsKey("endColumn")); + } + } + [Fact] [Trait("Level", "L0")] [Trait("Category", "Worker")] @@ -268,6 +346,7 @@ namespace GitHub.Runner.Common.Tests.Worker new EchoCommandExtension(), new InternalPluginSetRepoPathCommandExtension(), new SetEnvCommandExtension(), + new WarningCommandExtension(), }; foreach (var command in commands) { @@ -285,6 +364,10 @@ namespace GitHub.Runner.Common.Tests.Worker _ec = new Mock(); _ec.SetupAllProperties(); _ec.Setup(x => x.Global).Returns(new GlobalContext()); + _ec.Object.Global.Variables = new Variables( + hostContext, + new Dictionary() + ); // Command manager _commandManager = new ActionCommandManager();