From 3902257e9df894dcad84ac2d508d0fc2b2d5e252 Mon Sep 17 00:00:00 2001 From: John Wesley Walker III <81404201+jww3@users.noreply.github.com> Date: Mon, 16 Jan 2023 23:34:26 +0000 Subject: [PATCH 1/9] Make logging behavior optional in ExecutionContext::AddIssue --- src/Runner.Worker/ActionCommandManager.cs | 8 +- src/Runner.Worker/ExecutionContext.cs | 98 +++++++------------ src/Runner.Worker/Handlers/OutputManager.cs | 7 +- src/Runner.Worker/JobExtension.cs | 8 +- src/Runner.Worker/JobRunner.cs | 3 +- .../Common/Utility/PrimitiveExtensions.cs | 14 ++- 6 files changed, 60 insertions(+), 78 deletions(-) diff --git a/src/Runner.Worker/ActionCommandManager.cs b/src/Runner.Worker/ActionCommandManager.cs index 33fab1f2141..5f0a7438c9c 100644 --- a/src/Runner.Worker/ActionCommandManager.cs +++ b/src/Runner.Worker/ActionCommandManager.cs @@ -276,7 +276,7 @@ public void ProcessCommand(IExecutionContext context, string line, ActionCommand Message = $"Can't update {blocked} environment variable using ::set-env:: command." }; issue.Data[Constants.Runner.InternalTelemetryIssueDataKey] = $"{Constants.Runner.UnsupportedCommand}_{envName}"; - context.AddIssue(issue); + context.AddIssue(issue, writeToLog: true); return; } @@ -315,7 +315,7 @@ public void ProcessCommand(IExecutionContext context, string line, ActionCommand Message = String.Format(Constants.Runner.UnsupportedCommandMessage, this.Command) }; issue.Data[Constants.Runner.InternalTelemetryIssueDataKey] = Constants.Runner.UnsupportedCommand; - context.AddIssue(issue); + context.AddIssue(issue, writeToLog: true); } if (!command.Properties.TryGetValue(SetOutputCommandProperties.Name, out string outputName) || string.IsNullOrEmpty(outputName)) @@ -350,7 +350,7 @@ public void ProcessCommand(IExecutionContext context, string line, ActionCommand Message = String.Format(Constants.Runner.UnsupportedCommandMessage, this.Command) }; issue.Data[Constants.Runner.InternalTelemetryIssueDataKey] = Constants.Runner.UnsupportedCommand; - context.AddIssue(issue); + context.AddIssue(issue, writeToLog: true); } if (!command.Properties.TryGetValue(SaveStateCommandProperties.Name, out string stateName) || string.IsNullOrEmpty(stateName)) @@ -666,7 +666,7 @@ public void ProcessCommand(IExecutionContext context, string inputLine, ActionCo } } - context.AddIssue(issue); + context.AddIssue(issue, writeToLog: true); } public static void ValidateLinesAndColumns(ActionCommand command, IExecutionContext context) diff --git a/src/Runner.Worker/ExecutionContext.cs b/src/Runner.Worker/ExecutionContext.cs index 9034a9b872d..e9e7dbf4f2b 100644 --- a/src/Runner.Worker/ExecutionContext.cs +++ b/src/Runner.Worker/ExecutionContext.cs @@ -16,6 +16,7 @@ using GitHub.Runner.Sdk; using GitHub.Runner.Worker.Container; using GitHub.Runner.Worker.Handlers; +using GitHub.Services.Common; using Newtonsoft.Json; using ObjectTemplating = GitHub.DistributedTask.ObjectTemplating; using Pipelines = GitHub.DistributedTask.Pipelines; @@ -90,7 +91,7 @@ public interface IExecutionContext : IRunnerService void SetGitHubContext(string name, string value); void SetOutput(string name, string value, out string reference); void SetTimeout(TimeSpan? timeout); - void AddIssue(Issue issue, string message = null); + void AddIssue(Issue issue, bool writeToLog); void Progress(int percentage, string currentOperation = null); void UpdateDetailTimelineRecord(TimelineRecord record); @@ -446,7 +447,7 @@ public TaskResult Complete(TaskResult? result = null, string currentOperation = { foreach (var issue in _embeddedIssueCollector) { - AddIssue(issue); + AddIssue(issue, writeToLog: false); } } @@ -577,73 +578,50 @@ public void Progress(int percentage, string currentOperation = null) _jobServerQueue.QueueTimelineRecordUpdate(_mainTimelineId, _record); } - // This is not thread safe, the caller need to take lock before calling issue() - public void AddIssue(Issue issue, string logMessage = null) + // This is not thread safe, the caller needs to take lock before calling issue() + public void AddIssue(Issue issue, bool writeToLog) { ArgUtil.NotNull(issue, nameof(issue)); - if (string.IsNullOrEmpty(logMessage)) - { - logMessage = issue.Message; - } - - issue.Message = HostContext.SecretMasker.MaskSecrets(issue.Message); - if (issue.Message.Length > _maxIssueMessageLength) - { - issue.Message = issue.Message[.._maxIssueMessageLength]; - } + string refinedMessage = PrimitiveExtensions.TrimExcess(HostContext.SecretMasker.MaskSecrets(issue.Message), _maxIssueMessageLength); + issue.Message = refinedMessage; - // Tracking the line number (logFileLineNumber) and step number (stepNumber) for each issue that gets created - // Actions UI from the run summary page use both values to easily link to an exact locations in logs where annotations originate from + // It's important to keep track of the step number (key:stepNumber) and the line number (key:logFileLineNumber) of every issue that gets logged. + // Actions UI from the run summary page use both values to easily link to an exact locations in logs where annotations originate from. if (_record.Order != null) { issue.Data["stepNumber"] = _record.Order.ToString(); } - if (issue.Type == IssueType.Error) + string wellKnownTag = null; + Int32? previousCountForIssueType = null; + switch (issue.Type) { - if (!string.IsNullOrEmpty(logMessage)) - { - long logLineNumber = Write(WellKnownTags.Error, logMessage); - issue.Data["logFileLineNumber"] = logLineNumber.ToString(); - } - - if (_record.ErrorCount < _maxIssueCount) - { - _record.Issues.Add(issue); - } - - _record.ErrorCount++; + case IssueType.Error: + wellKnownTag = WellKnownTags.Error; + previousCountForIssueType = _record.ErrorCount++; + break; + case IssueType.Warning: + wellKnownTag = WellKnownTags.Warning; + previousCountForIssueType = _record.WarningCount++; + break; + case IssueType.Notice: + wellKnownTag = WellKnownTags.Notice; + previousCountForIssueType = _record.NoticeCount++; + break; } - else if (issue.Type == IssueType.Warning) - { - if (!string.IsNullOrEmpty(logMessage)) - { - long logLineNumber = Write(WellKnownTags.Warning, logMessage); - issue.Data["logFileLineNumber"] = logLineNumber.ToString(); - } - if (_record.WarningCount < _maxIssueCount) - { - _record.Issues.Add(issue); - } - - _record.WarningCount++; - } - else if (issue.Type == IssueType.Notice) + if (!string.IsNullOrEmpty(wellKnownTag)) { - if (!string.IsNullOrEmpty(logMessage)) + if (writeToLog && !string.IsNullOrEmpty(refinedMessage)) { - long logLineNumber = Write(WellKnownTags.Notice, logMessage); + long logLineNumber = Write(wellKnownTag, refinedMessage); issue.Data["logFileLineNumber"] = logLineNumber.ToString(); } - - if (_record.NoticeCount < _maxIssueCount) + if (previousCountForIssueType.GetValueOrDefault(0) < _maxIssueCount) { _record.Issues.Add(issue); } - - _record.NoticeCount++; } // Embedded ExecutionContexts (a.k.a. Composite actions) should never upload a timeline record to the server. @@ -1017,16 +995,7 @@ public void PublishStepTelemetry() if ((issue.Type == IssueType.Error || issue.Type == IssueType.Warning) && !string.IsNullOrEmpty(issue.Message)) { - string issueTelemetry; - if (issue.Message.Length > _maxIssueMessageLengthInTelemetry) - { - issueTelemetry = $"{issue.Message[.._maxIssueMessageLengthInTelemetry]}"; - } - else - { - issueTelemetry = issue.Message; - } - + string issueTelemetry = PrimitiveExtensions.TrimExcess(issue.Message, _maxIssueMessageLengthInTelemetry); StepTelemetry.ErrorMessages.Add(issueTelemetry); // Only send over the first 3 issues to avoid sending too much data. @@ -1200,19 +1169,22 @@ public static void Error(this IExecutionContext context, Exception ex) // Do not add a format string overload. See comment on ExecutionContext.Write(). public static void Error(this IExecutionContext context, string message) { - context.AddIssue(new Issue() { Type = IssueType.Error, Message = message }); + var issue = new Issue() { Type = IssueType.Error, Message = message }; + context.AddIssue(issue, writeToLog: true); } // Do not add a format string overload. See comment on ExecutionContext.Write(). public static void InfrastructureError(this IExecutionContext context, string message) { - context.AddIssue(new Issue() { Type = IssueType.Error, Message = message, IsInfrastructureIssue = true }); + var issue = new Issue() { Type = IssueType.Error, Message = message, IsInfrastructureIssue = true }; + context.AddIssue(issue, writeToLog: true); } // Do not add a format string overload. See comment on ExecutionContext.Write(). public static void Warning(this IExecutionContext context, string message) { - context.AddIssue(new Issue() { Type = IssueType.Warning, Message = message }); + var issue = new Issue() { Type = IssueType.Warning, Message = message }; + context.AddIssue(issue, writeToLog: true); } // Do not add a format string overload. See comment on ExecutionContext.Write(). diff --git a/src/Runner.Worker/Handlers/OutputManager.cs b/src/Runner.Worker/Handlers/OutputManager.cs index 8b97c867d95..dc683422f30 100644 --- a/src/Runner.Worker/Handlers/OutputManager.cs +++ b/src/Runner.Worker/Handlers/OutputManager.cs @@ -108,7 +108,6 @@ public void OnDataReceived(object sender, ProcessDataReceivedEventArgs e) try { match = matcher.Match(stripped); - break; } catch (RegexMatchTimeoutException ex) @@ -116,7 +115,7 @@ public void OnDataReceived(object sender, ProcessDataReceivedEventArgs e) if (attempt < _maxAttempts) { // Debug - _executionContext.Debug($"Timeout processing issue matcher '{matcher.Owner}' against line '{stripped}'. Exception: {ex.ToString()}"); + _executionContext.Debug($"Timeout processing issue matcher '{matcher.Owner}' against line '{stripped}'. Exception: {ex}"); } else { @@ -139,12 +138,10 @@ public void OnDataReceived(object sender, ProcessDataReceivedEventArgs e) // Convert to issue var issue = ConvertToIssue(match); - if (issue != null) { // Log issue - _executionContext.AddIssue(issue, stripped); - + _executionContext.AddIssue(issue, writeToLog: true); return; } } diff --git a/src/Runner.Worker/JobExtension.cs b/src/Runner.Worker/JobExtension.cs index 246274734d9..ec91510e112 100644 --- a/src/Runner.Worker/JobExtension.cs +++ b/src/Runner.Worker/JobExtension.cs @@ -325,10 +325,10 @@ public async Task> InitializeJob(IExecutionContext jobContext, Pipel if (message.ContextData.TryGetValue("inputs", out var pipelineContextData)) { var inputs = pipelineContextData.AssertDictionary("inputs"); - if (inputs.Any()) + if (inputs.Any()) { context.Output($"##[group] Inputs"); - foreach (var input in inputs) + foreach (var input in inputs) { context.Output($" {input.Key}: {input.Value}"); } @@ -336,7 +336,7 @@ public async Task> InitializeJob(IExecutionContext jobContext, Pipel } } - if (!string.IsNullOrWhiteSpace(message.JobDisplayName)) + if (!string.IsNullOrWhiteSpace(message.JobDisplayName)) { context.Output($"Complete job name: {message.JobDisplayName}"); } @@ -662,7 +662,7 @@ private async Task CheckDiskSpaceAsync(IExecutionContext context, CancellationTo { var issue = new Issue() { Type = IssueType.Warning, Message = $"You are running out of disk space. The runner will stop working when the machine runs out of disk space. Free space left: {freeSpaceInMB} MB" }; issue.Data[Constants.Runner.InternalTelemetryIssueDataKey] = Constants.Runner.LowDiskSpace; - context.AddIssue(issue); + context.AddIssue(issue, writeToLog: true); return; } diff --git a/src/Runner.Worker/JobRunner.cs b/src/Runner.Worker/JobRunner.cs index 562ef9068fd..fca6c72e9ab 100644 --- a/src/Runner.Worker/JobRunner.cs +++ b/src/Runner.Worker/JobRunner.cs @@ -84,7 +84,8 @@ public async Task RunAsync(Pipelines.AgentJobRequestMessage message, default: throw new ArgumentException(HostContext.RunnerShutdownReason.ToString(), nameof(HostContext.RunnerShutdownReason)); } - jobContext.AddIssue(new Issue() { Type = IssueType.Error, Message = errorMessage }); + var issue = new Issue() { Type = IssueType.Error, Message = errorMessage }; + jobContext.AddIssue(issue, writeToLog: true); }); // Validate directory permissions. diff --git a/src/Sdk/Common/Common/Utility/PrimitiveExtensions.cs b/src/Sdk/Common/Common/Utility/PrimitiveExtensions.cs index f15bf502b67..aae3824899a 100644 --- a/src/Sdk/Common/Common/Utility/PrimitiveExtensions.cs +++ b/src/Sdk/Common/Common/Utility/PrimitiveExtensions.cs @@ -27,6 +27,18 @@ public static DateTime FromUnixEpochTime(this Int64 unixTime) } } + public static string TrimExcess(string text, int maxLength) + { + string result = text; + + if (!string.IsNullOrEmpty(text) && text.Length > maxLength) + { + result = text.Substring(0, maxLength); + } + + return result; + } + public static string ToBase64StringNoPaddingFromString(string utf8String) { return ToBase64StringNoPadding(Encoding.UTF8.GetBytes(utf8String)); @@ -46,7 +58,7 @@ public static string FromBase64StringNoPaddingToString(string base64String) //These methods convert To and From base64 strings without padding //for JWT scenarios - //code taken from the JWS spec here: + //code taken from the JWS spec here: //http://tools.ietf.org/html/draft-ietf-jose-json-web-signature-08#appendix-C public static String ToBase64StringNoPadding(this byte[] bytes) { From 87ababb8586d12fc4094e0b97936e95fd9001aaf Mon Sep 17 00:00:00 2001 From: John Wesley Walker III <81404201+jww3@users.noreply.github.com> Date: Tue, 17 Jan 2023 11:36:51 +0000 Subject: [PATCH 2/9] Pivot toward Immutable DistributedTask.WebApi::Issue instances. The proper way to instantiate a DistributedTask.WebApi::Issue is now via the factory method on an ExecutionContext (IExecutionContext::CreateIssue) CreateIssue factory. --- src/Runner.Worker/ActionCommandManager.cs | 56 ++---- src/Runner.Worker/ExecutionContext.cs | 51 +++-- src/Runner.Worker/Handlers/OutputManager.cs | 23 ++- src/Runner.Worker/JobExtension.cs | 7 +- src/Runner.Worker/JobRunner.cs | 4 +- .../Common/Utility/PrimitiveExtensions.cs | 4 +- src/Sdk/DTWebApi/WebApi/Issue.cs | 57 +++++- src/Test/L0/Worker/ActionCommandManagerL0.cs | 33 +++- src/Test/L0/Worker/ActionManagerL0.cs | 17 +- src/Test/L0/Worker/ActionManifestManagerL0.cs | 23 ++- src/Test/L0/Worker/ActionRunnerL0.cs | 25 ++- .../L0/Worker/CreateStepSummaryCommandL0.cs | 28 ++- src/Test/L0/Worker/ExecutionContextL0.cs | 102 +++++----- src/Test/L0/Worker/OutputManagerL0.cs | 178 ++++++++++-------- src/Test/L0/Worker/SaveStateFileCommandL0.cs | 13 +- src/Test/L0/Worker/SetEnvFileCommandL0.cs | 19 +- src/Test/L0/Worker/SetOutputFileCommandL0.cs | 19 +- 17 files changed, 383 insertions(+), 276 deletions(-) diff --git a/src/Runner.Worker/ActionCommandManager.cs b/src/Runner.Worker/ActionCommandManager.cs index 5f0a7438c9c..42230588b7e 100644 --- a/src/Runner.Worker/ActionCommandManager.cs +++ b/src/Runner.Worker/ActionCommandManager.cs @@ -270,13 +270,10 @@ public void ProcessCommand(IExecutionContext context, string line, ActionCommand if (string.Equals(blocked, envName, StringComparison.OrdinalIgnoreCase)) { // Log Telemetry and let user know they shouldn't do this - var issue = new Issue() - { - Type = IssueType.Error, - Message = $"Can't update {blocked} environment variable using ::set-env:: command." - }; - issue.Data[Constants.Runner.InternalTelemetryIssueDataKey] = $"{Constants.Runner.UnsupportedCommand}_{envName}"; - context.AddIssue(issue, writeToLog: true); + var message = $"Can't update {blocked} environment variable using ::set-env:: command."; + var metadata = new IssueMetadata(Constants.Runner.InternalTelemetryIssueDataKey, $"{Constants.Runner.UnsupportedCommand}_{envName}"); + var issue = context.CreateIssue(IssueType.Error, message, metadata, true); + context.AddIssue(issue); return; } @@ -309,13 +306,10 @@ public void ProcessCommand(IExecutionContext context, string line, ActionCommand { if (context.Global.Variables.GetBoolean("DistributedTask.DeprecateStepOutputCommands") ?? false) { - var issue = new Issue() - { - Type = IssueType.Warning, - Message = String.Format(Constants.Runner.UnsupportedCommandMessage, this.Command) - }; - issue.Data[Constants.Runner.InternalTelemetryIssueDataKey] = Constants.Runner.UnsupportedCommand; - context.AddIssue(issue, writeToLog: true); + var message = string.Format(Constants.Runner.UnsupportedCommandMessage, this.Command); + var metadata = new IssueMetadata(Constants.Runner.InternalTelemetryIssueDataKey, Constants.Runner.UnsupportedCommand); + var issue = context.CreateIssue(IssueType.Warning, message, metadata, true); + context.AddIssue(issue); } if (!command.Properties.TryGetValue(SetOutputCommandProperties.Name, out string outputName) || string.IsNullOrEmpty(outputName)) @@ -344,13 +338,10 @@ public void ProcessCommand(IExecutionContext context, string line, ActionCommand { if (context.Global.Variables.GetBoolean("DistributedTask.DeprecateStepOutputCommands") ?? false) { - var issue = new Issue() - { - Type = IssueType.Warning, - Message = String.Format(Constants.Runner.UnsupportedCommandMessage, this.Command) - }; - issue.Data[Constants.Runner.InternalTelemetryIssueDataKey] = Constants.Runner.UnsupportedCommand; - context.AddIssue(issue, writeToLog: true); + var message = string.Format(Constants.Runner.UnsupportedCommandMessage, this.Command); + var metadata = new IssueMetadata(Constants.Runner.InternalTelemetryIssueDataKey, Constants.Runner.UnsupportedCommand); + var issue = context.CreateIssue(IssueType.Warning, message, metadata, true); + context.AddIssue(issue); } if (!command.Properties.TryGetValue(SaveStateCommandProperties.Name, out string stateName) || string.IsNullOrEmpty(stateName)) @@ -618,16 +609,11 @@ public void ProcessCommand(IExecutionContext context, string inputLine, ActionCo context.Debug("Enhanced Annotations not enabled on the server. The 'title', 'end_line', and 'end_column' fields are unsupported."); } - Issue issue = new() - { - Category = "General", - Type = this.Type, - Message = command.Data - }; + var issueCategory = "General"; if (!string.IsNullOrEmpty(file)) { - issue.Category = "Code"; + issueCategory = "Code"; if (container != null) { @@ -658,15 +644,13 @@ public void ProcessCommand(IExecutionContext context, string inputLine, ActionCo } } - foreach (var property in command.Properties) - { - if (!string.Equals(property.Key, Constants.Runner.InternalTelemetryIssueDataKey, StringComparison.OrdinalIgnoreCase)) - { - issue.Data[property.Key] = property.Value; - } - } + string keyToExclude = Constants.Runner.InternalTelemetryIssueDataKey; + var filteredDictionaryEntries = command.Properties + .Where(kvp => !string.Equals(kvp.Key, keyToExclude, StringComparison.OrdinalIgnoreCase)); - context.AddIssue(issue, writeToLog: true); + var metadata = new IssueMetadata(issueCategory, false, filteredDictionaryEntries); + var issue = context.CreateIssue(this.Type, command.Data, metadata, true); + context.AddIssue(issue); } public static void ValidateLinesAndColumns(ActionCommand command, IExecutionContext context) diff --git a/src/Runner.Worker/ExecutionContext.cs b/src/Runner.Worker/ExecutionContext.cs index e9e7dbf4f2b..ec59b029d49 100644 --- a/src/Runner.Worker/ExecutionContext.cs +++ b/src/Runner.Worker/ExecutionContext.cs @@ -91,7 +91,8 @@ public interface IExecutionContext : IRunnerService void SetGitHubContext(string name, string value); void SetOutput(string name, string value, out string reference); void SetTimeout(TimeSpan? timeout); - void AddIssue(Issue issue, bool writeToLog); + IReadOnlyIssue CreateIssue(IssueType issueType, string rawMessage, IssueMetadata metadata, bool writeToLog); + void AddIssue(IReadOnlyIssue issue); void Progress(int percentage, string currentOperation = null); void UpdateDetailTimelineRecord(TimelineRecord record); @@ -125,7 +126,7 @@ public sealed class ExecutionContext : RunnerService, IExecutionContext private readonly TimelineRecord _record = new(); private readonly Dictionary _detailRecords = new(); - private readonly List _embeddedIssueCollector; + private readonly List _embeddedIssueCollector; private readonly object _loggerLock = new(); private readonly object _matchersLock = new(); private readonly ExecutionContext _parentExecutionContext; @@ -447,7 +448,7 @@ public TaskResult Complete(TaskResult? result = null, string currentOperation = { foreach (var issue in _embeddedIssueCollector) { - AddIssue(issue, writeToLog: false); + AddIssue(issue); } } @@ -579,23 +580,29 @@ public void Progress(int percentage, string currentOperation = null) } // This is not thread safe, the caller needs to take lock before calling issue() - public void AddIssue(Issue issue, bool writeToLog) + public IReadOnlyIssue CreateIssue(IssueType issueType, string rawMessage, IssueMetadata metadata, bool writeToLog) { - ArgUtil.NotNull(issue, nameof(issue)); + string refinedMessage = PrimitiveExtensions.TrimExcess(HostContext.SecretMasker.MaskSecrets(rawMessage), _maxIssueMessageLength); + + var result = new Issue() { + Type = issueType, + Category = metadata?.Category, + IsInfrastructureIssue = metadata?.IsInfrastructureIssue ?? false, + Message = refinedMessage, + }; - string refinedMessage = PrimitiveExtensions.TrimExcess(HostContext.SecretMasker.MaskSecrets(issue.Message), _maxIssueMessageLength); - issue.Message = refinedMessage; + result.Data.AddRangeIfRangeNotNull(metadata?.Data); // It's important to keep track of the step number (key:stepNumber) and the line number (key:logFileLineNumber) of every issue that gets logged. // Actions UI from the run summary page use both values to easily link to an exact locations in logs where annotations originate from. if (_record.Order != null) { - issue.Data["stepNumber"] = _record.Order.ToString(); + result.Data["stepNumber"] = _record.Order.ToString(); } string wellKnownTag = null; Int32? previousCountForIssueType = null; - switch (issue.Type) + switch (issueType) { case IssueType.Error: wellKnownTag = WellKnownTags.Error; @@ -616,14 +623,23 @@ public void AddIssue(Issue issue, bool writeToLog) if (writeToLog && !string.IsNullOrEmpty(refinedMessage)) { long logLineNumber = Write(wellKnownTag, refinedMessage); - issue.Data["logFileLineNumber"] = logLineNumber.ToString(); + result.Data["logFileLineNumber"] = logLineNumber.ToString(); } if (previousCountForIssueType.GetValueOrDefault(0) < _maxIssueCount) { - _record.Issues.Add(issue); + _record.Issues.Add(result); } } + return result; + } + + + // This is not thread safe, the caller needs to take lock before calling issue() + public void AddIssue(IReadOnlyIssue issue) + { + ArgUtil.NotNull(issue, nameof(issue)); + // Embedded ExecutionContexts (a.k.a. Composite actions) should never upload a timeline record to the server. // Instead, we store processed issues on a shared (psuedo-inherited) list (belonging to the closest // non-embedded ancestor ExecutionContext) so that they can be processed when that ancestor completes. @@ -1169,22 +1185,23 @@ public static void Error(this IExecutionContext context, Exception ex) // Do not add a format string overload. See comment on ExecutionContext.Write(). public static void Error(this IExecutionContext context, string message) { - var issue = new Issue() { Type = IssueType.Error, Message = message }; - context.AddIssue(issue, writeToLog: true); + var issue = context.CreateIssue(IssueType.Error, message, null, true); + context.AddIssue(issue); } // Do not add a format string overload. See comment on ExecutionContext.Write(). public static void InfrastructureError(this IExecutionContext context, string message) { - var issue = new Issue() { Type = IssueType.Error, Message = message, IsInfrastructureIssue = true }; - context.AddIssue(issue, writeToLog: true); + var metadata = new IssueMetadata(null, true, Enumerable.Empty>()); + var issue = context.CreateIssue(IssueType.Error, message, metadata, true); + context.AddIssue(issue); } // Do not add a format string overload. See comment on ExecutionContext.Write(). public static void Warning(this IExecutionContext context, string message) { - var issue = new Issue() { Type = IssueType.Warning, Message = message }; - context.AddIssue(issue, writeToLog: true); + var issue = context.CreateIssue(IssueType.Warning, message, null, true); + context.AddIssue(issue); } // Do not add a format string overload. See comment on ExecutionContext.Write(). diff --git a/src/Runner.Worker/Handlers/OutputManager.cs b/src/Runner.Worker/Handlers/OutputManager.cs index dc683422f30..3f5301e15d9 100644 --- a/src/Runner.Worker/Handlers/OutputManager.cs +++ b/src/Runner.Worker/Handlers/OutputManager.cs @@ -4,6 +4,7 @@ using System.IO; using System.Linq; using System.Text.RegularExpressions; +using GitHub.DistributedTask.WebApi; using GitHub.Runner.Common; using GitHub.Runner.Sdk; using GitHub.Runner.Worker.Container; @@ -141,7 +142,7 @@ public void OnDataReceived(object sender, ProcessDataReceivedEventArgs e) if (issue != null) { // Log issue - _executionContext.AddIssue(issue, writeToLog: true); + _executionContext.AddIssue(issue); return; } } @@ -193,7 +194,7 @@ private void Remove(IssueMatcher matcher) } } - private DTWebApi.Issue ConvertToIssue(IssueMatch match) + private DTWebApi.IReadOnlyIssue ConvertToIssue(IssueMatch match) { // Validate the message if (string.IsNullOrWhiteSpace(match.Message)) @@ -222,18 +223,14 @@ private DTWebApi.Issue ConvertToIssue(IssueMatch match) return null; } - var issue = new DTWebApi.Issue - { - Message = match.Message, - Type = issueType, - }; + var issueData = new Dictionary(); // Line if (!string.IsNullOrEmpty(match.Line)) { if (int.TryParse(match.Line, NumberStyles.None, CultureInfo.InvariantCulture, out var line)) { - issue.Data["line"] = line.ToString(CultureInfo.InvariantCulture); + issueData["line"] = line.ToString(CultureInfo.InvariantCulture); } else { @@ -246,7 +243,7 @@ private DTWebApi.Issue ConvertToIssue(IssueMatch match) { if (int.TryParse(match.Column, NumberStyles.None, CultureInfo.InvariantCulture, out var column)) { - issue.Data["col"] = column.ToString(CultureInfo.InvariantCulture); + issueData["col"] = column.ToString(CultureInfo.InvariantCulture); } else { @@ -257,7 +254,7 @@ private DTWebApi.Issue ConvertToIssue(IssueMatch match) // Code if (!string.IsNullOrWhiteSpace(match.Code)) { - issue.Data["code"] = match.Code.Trim(); + issueData["code"] = match.Code.Trim(); } // File @@ -309,7 +306,7 @@ private DTWebApi.Issue ConvertToIssue(IssueMatch match) var relativePath = file.Substring(repositoryPath.Length).TrimStart(Path.DirectorySeparatorChar); // Prefer `/` on all platforms - issue.Data["file"] = relativePath.Replace(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar); + issueData["file"] = relativePath.Replace(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar); } else { @@ -324,9 +321,11 @@ private DTWebApi.Issue ConvertToIssue(IssueMatch match) } catch (Exception ex) { - _executionContext.Debug($"Dropping file value '{match.File}' and fromPath value '{match.FromPath}'. Exception during validation: {ex.ToString()}"); + _executionContext.Debug($"Dropping file value '{match.File}' and fromPath value '{match.FromPath}'. Exception during validation: {ex}"); } + var metadata = new IssueMetadata(null, false, issueData); + var issue = _executionContext.CreateIssue(issueType, match.Message, metadata, true); return issue; } diff --git a/src/Runner.Worker/JobExtension.cs b/src/Runner.Worker/JobExtension.cs index ec91510e112..52f039d3185 100644 --- a/src/Runner.Worker/JobExtension.cs +++ b/src/Runner.Worker/JobExtension.cs @@ -660,9 +660,10 @@ private async Task CheckDiskSpaceAsync(IExecutionContext context, CancellationTo var freeSpaceInMB = driveInfo.AvailableFreeSpace / 1024 / 1024; if (freeSpaceInMB < lowDiskSpaceThreshold) { - var issue = new Issue() { Type = IssueType.Warning, Message = $"You are running out of disk space. The runner will stop working when the machine runs out of disk space. Free space left: {freeSpaceInMB} MB" }; - issue.Data[Constants.Runner.InternalTelemetryIssueDataKey] = Constants.Runner.LowDiskSpace; - context.AddIssue(issue, writeToLog: true); + var message = $"You are running out of disk space. The runner will stop working when the machine runs out of disk space. Free space left: {freeSpaceInMB} MB"; + var metadata = new IssueMetadata(Constants.Runner.InternalTelemetryIssueDataKey, Constants.Runner.LowDiskSpace); + var issue = context.CreateIssue(IssueType.Warning, message, metadata, true); + context.AddIssue(issue); return; } diff --git a/src/Runner.Worker/JobRunner.cs b/src/Runner.Worker/JobRunner.cs index fca6c72e9ab..b5a1bcc256e 100644 --- a/src/Runner.Worker/JobRunner.cs +++ b/src/Runner.Worker/JobRunner.cs @@ -84,8 +84,8 @@ public async Task RunAsync(Pipelines.AgentJobRequestMessage message, default: throw new ArgumentException(HostContext.RunnerShutdownReason.ToString(), nameof(HostContext.RunnerShutdownReason)); } - var issue = new Issue() { Type = IssueType.Error, Message = errorMessage }; - jobContext.AddIssue(issue, writeToLog: true); + var issue = jobContext.CreateIssue(IssueType.Error, errorMessage, null, true); + jobContext.AddIssue(issue); }); // Validate directory permissions. diff --git a/src/Sdk/Common/Common/Utility/PrimitiveExtensions.cs b/src/Sdk/Common/Common/Utility/PrimitiveExtensions.cs index aae3824899a..4b00293c702 100644 --- a/src/Sdk/Common/Common/Utility/PrimitiveExtensions.cs +++ b/src/Sdk/Common/Common/Utility/PrimitiveExtensions.cs @@ -31,9 +31,9 @@ public static string TrimExcess(string text, int maxLength) { string result = text; - if (!string.IsNullOrEmpty(text) && text.Length > maxLength) + if (!string.IsNullOrEmpty(result) && result.Length > maxLength) { - result = text.Substring(0, maxLength); + result = result.Substring(0, maxLength); } return result; diff --git a/src/Sdk/DTWebApi/WebApi/Issue.cs b/src/Sdk/DTWebApi/WebApi/Issue.cs index af4d2f85034..d68815402ac 100644 --- a/src/Sdk/DTWebApi/WebApi/Issue.cs +++ b/src/Sdk/DTWebApi/WebApi/Issue.cs @@ -4,24 +4,55 @@ namespace GitHub.DistributedTask.WebApi { + + public interface IReadOnlyIssue + { + IssueType Type { get; } + string Category { get; } + string Message { get; } + bool? IsInfrastructureIssue { get; } + string this[string key] { get; } + } + + public class IssueMetadata + { + + public IssueMetadata(string key, string value) + : this(null, false, new []{ KeyValuePair.Create(key, value) }) + { + } + + public IssueMetadata(string category, bool infrastructureIssue, IEnumerable> data) + { + this.Category = category; + this.IsInfrastructureIssue = infrastructureIssue; + this.Data = data; + } + + + public readonly string Category; + public readonly bool IsInfrastructureIssue; + public readonly IEnumerable> Data; + } + [DataContract] - public class Issue + public class Issue : IReadOnlyIssue { public Issue() { } - private Issue(Issue issueToBeCloned) + private Issue(Issue original) { - this.Type = issueToBeCloned.Type; - this.Category = issueToBeCloned.Category; - this.Message = issueToBeCloned.Message; - this.IsInfrastructureIssue = issueToBeCloned.IsInfrastructureIssue; + this.Type = original.Type; + this.Category = original.Category; + this.Message = original.Message; + this.IsInfrastructureIssue = original.IsInfrastructureIssue; - if (issueToBeCloned.m_data != null) + if (original.m_data != null) { - foreach (var item in issueToBeCloned.m_data) + foreach (var item in original.m_data) { this.Data.Add(item); } @@ -68,6 +99,16 @@ public IDictionary Data } } + public string this[string key] + { + get + { + m_data.TryGetValue(key, out string result); + return result; + } + } + + public Issue Clone() { return new Issue(this); diff --git a/src/Test/L0/Worker/ActionCommandManagerL0.cs b/src/Test/L0/Worker/ActionCommandManagerL0.cs index cef826fd94e..a94413cb8a3 100644 --- a/src/Test/L0/Worker/ActionCommandManagerL0.cs +++ b/src/Test/L0/Worker/ActionCommandManagerL0.cs @@ -6,6 +6,7 @@ using GitHub.DistributedTask.WebApi; using GitHub.Runner.Worker; using GitHub.Runner.Worker.Container; +using GitHub.Services.Common; using Moq; using Xunit; using Pipelines = GitHub.DistributedTask.Pipelines; @@ -32,10 +33,10 @@ public void EnablePluginInternalCommand() hc.GetTrace().Info($"{tag} {line}"); return 1; }); - _ec.Setup(x => x.AddIssue(It.IsAny(), It.IsAny())) - .Callback((Issue issue, string message) => + _ec.Setup(x => x.AddIssue(It.IsAny())) + .Callback((IReadOnlyIssue issue) => { - hc.GetTrace().Info($"{issue.Type} {issue.Message} {message ?? string.Empty}"); + hc.GetTrace().Info($"{issue.Type} {issue.Message}"); }); _commandManager.EnablePluginInternalCommand(); @@ -59,10 +60,10 @@ public void DisablePluginInternalCommand() hc.GetTrace().Info($"{tag} {line}"); return 1; }); - _ec.Setup(x => x.AddIssue(It.IsAny(), It.IsAny())) - .Callback((Issue issue, string message) => + _ec.Setup(x => x.AddIssue(It.IsAny())) + .Callback((IReadOnlyIssue issue) => { - hc.GetTrace().Info($"{issue.Type} {issue.Message} {message ?? string.Empty}"); + hc.GetTrace().Info($"{issue.Type} {issue.Message}"); }); _commandManager.EnablePluginInternalCommand(); @@ -92,10 +93,24 @@ public void StopProcessCommand() return 1; }); - _ec.Setup(x => x.AddIssue(It.IsAny(), It.IsAny())) - .Callback((Issue issue, string message) => + _ec.Setup(x => x.CreateIssue(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) + .Returns((IssueType type, string message, IssueMetadata metadata, bool writeToLog) => + { + var result = new Issue() + { + Type = type, + Message = message, + Category = metadata?.Category, + IsInfrastructureIssue = metadata?.IsInfrastructureIssue ?? false + }; + result.Data.AddRangeIfRangeNotNull(metadata?.Data); + return result; + }); + + _ec.Setup(x => x.AddIssue(It.IsAny())) + .Callback((IReadOnlyIssue issue) => { - hc.GetTrace().Info($"{issue.Type} {issue.Message} {message ?? string.Empty}"); + hc.GetTrace().Info($"{issue.Type} {issue.Message}"); }); _ec.Object.Global.EnvironmentVariables = new Dictionary(); diff --git a/src/Test/L0/Worker/ActionManagerL0.cs b/src/Test/L0/Worker/ActionManagerL0.cs index 07cf9b48700..1d14bf5ed5a 100644 --- a/src/Test/L0/Worker/ActionManagerL0.cs +++ b/src/Test/L0/Worker/ActionManagerL0.cs @@ -1,7 +1,6 @@ using System; using System.Collections.Generic; using System.IO; -using System.IO.Compression; using System.Net; using System.Net.Http; using System.Runtime.CompilerServices; @@ -14,6 +13,7 @@ using GitHub.Runner.Sdk; using GitHub.Runner.Worker; using GitHub.Runner.Worker.Container; +using GitHub.Services.Common; using Moq; using Moq.Protected; using Xunit; @@ -2147,7 +2147,20 @@ private void Setup([CallerMemberName] string name = "", bool enableComposite = t _ec.Object.Global.FileTable = new List(); _ec.Object.Global.Plan = new TaskOrchestrationPlanReference(); _ec.Setup(x => x.Write(It.IsAny(), It.IsAny())).Callback((string tag, string message) => { _hc.GetTrace().Info($"[{tag}]{message}"); }); - _ec.Setup(x => x.AddIssue(It.IsAny(), It.IsAny())).Callback((Issue issue, string message) => { _hc.GetTrace().Info($"[{issue.Type}]{issue.Message ?? message}"); }); + _ec.Setup(x => x.CreateIssue(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) + .Returns((IssueType type, string message, IssueMetadata metadata, bool writeToLog) => + { + var result = new Issue() + { + Type = type, + Message = message, + Category = metadata?.Category, + IsInfrastructureIssue = metadata?.IsInfrastructureIssue ?? false + }; + result.Data.AddRangeIfRangeNotNull(metadata?.Data); + return result; + }); + _ec.Setup(x => x.AddIssue(It.IsAny())).Callback((IReadOnlyIssue issue) => { _hc.GetTrace().Info($"[{issue.Type}]{issue.Message}"); }); _ec.Setup(x => x.GetGitHubContext("workspace")).Returns(Path.Combine(_workFolder, "actions", "actions")); _dockerManager = new Mock(); diff --git a/src/Test/L0/Worker/ActionManifestManagerL0.cs b/src/Test/L0/Worker/ActionManifestManagerL0.cs index b6e3f855b34..aaec5f772dd 100644 --- a/src/Test/L0/Worker/ActionManifestManagerL0.cs +++ b/src/Test/L0/Worker/ActionManifestManagerL0.cs @@ -4,10 +4,10 @@ using GitHub.DistributedTask.WebApi; using GitHub.Runner.Worker; using GitHub.Runner.Worker.Expressions; +using GitHub.Services.Common; using Moq; using System; using System.Collections.Generic; -using System.Diagnostics; using System.IO; using System.Runtime.CompilerServices; using System.Threading; @@ -670,7 +670,7 @@ public void Load_PluginAction() { Teardown(); } - } + } [Fact] [Trait("Level", "L0")] @@ -715,7 +715,7 @@ public void Load_CompositeActionNoUsing() //Assert var err = Assert.Throws(() => actionManifest.Load(_ec.Object, action_path)); Assert.Contains($"Fail to load {action_path}", err.Message); - _ec.Verify(x => x.AddIssue(It.Is(s => s.Message.Contains("Missing 'using' value. 'using' requires 'composite', 'docker', 'node12' or 'node16'.")), It.IsAny()), Times.Once); + _ec.Verify(x => x.AddIssue(It.Is(s => s.Message.Contains("Missing 'using' value. 'using' requires 'composite', 'docker', 'node12' or 'node16'."))), Times.Once); } finally { @@ -860,7 +860,22 @@ private void Setup([CallerMemberName] string name = "") _ec.Setup(x => x.ExpressionValues).Returns(new DictionaryContextData()); _ec.Setup(x => x.ExpressionFunctions).Returns(new List()); _ec.Setup(x => x.Write(It.IsAny(), It.IsAny())).Callback((string tag, string message) => { _hc.GetTrace().Info($"{tag}{message}"); }); - _ec.Setup(x => x.AddIssue(It.IsAny(), It.IsAny())).Callback((Issue issue, string message) => { _hc.GetTrace().Info($"[{issue.Type}]{issue.Message ?? message}"); }); + + _ec.Setup(x => x.CreateIssue(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) + .Returns((IssueType type, string message, IssueMetadata metadata, bool writeToLog) => + { + var result = new Issue() + { + Type = type, + Message = message, + Category = metadata?.Category, + IsInfrastructureIssue = metadata?.IsInfrastructureIssue ?? false + }; + result.Data.AddRangeIfRangeNotNull(metadata?.Data); + return result; + }); + + _ec.Setup(x => x.AddIssue(It.IsAny())).Callback((IReadOnlyIssue issue) => { _hc.GetTrace().Info($"[{issue.Type}]{issue.Message}"); }); } private void Teardown() diff --git a/src/Test/L0/Worker/ActionRunnerL0.cs b/src/Test/L0/Worker/ActionRunnerL0.cs index 31d0743659b..f56f8553699 100644 --- a/src/Test/L0/Worker/ActionRunnerL0.cs +++ b/src/Test/L0/Worker/ActionRunnerL0.cs @@ -3,20 +3,15 @@ using GitHub.DistributedTask.Pipelines; using GitHub.DistributedTask.Pipelines.ContextData; using GitHub.DistributedTask.WebApi; -using GitHub.Runner.Common.Util; using GitHub.Runner.Worker; -using GitHub.Runner.Worker.Container; using GitHub.Runner.Worker.Handlers; +using GitHub.Services.Common; using Moq; using Newtonsoft.Json.Linq; using System; using System.Collections.Generic; -using System.IO; -using System.IO.Compression; -using System.Reflection; using System.Runtime.CompilerServices; using System.Threading; -using System.Threading.Tasks; using Xunit; using Pipelines = GitHub.DistributedTask.Pipelines; @@ -330,7 +325,7 @@ public async void WarnInvalidInputs() Assert.Equal("invalid1", finialInputs["invalid1"]); Assert.Equal("invalid2", finialInputs["invalid2"]); - _ec.Verify(x => x.AddIssue(It.Is(s => s.Message.Contains("Unexpected input(s) 'invalid1', 'invalid2'")), It.IsAny()), Times.Once); + _ec.Verify(x => x.AddIssue(It.Is(s => s.Message.Contains("Unexpected input(s) 'invalid1', 'invalid2'"))), Times.Once); } [Fact] @@ -449,7 +444,21 @@ private void Setup([CallerMemberName] string name = "") _ec.Setup(x => x.CancellationToken).Returns(_ecTokenSource.Token); _ec.Object.Global.Variables = new Variables(_hc, new Dictionary()); _ec.Setup(x => x.Write(It.IsAny(), It.IsAny())).Callback((string tag, string message) => { _hc.GetTrace().Info($"[{tag}]{message}"); }); - _ec.Setup(x => x.AddIssue(It.IsAny(), It.IsAny())).Callback((Issue issue, string message) => { _hc.GetTrace().Info($"[{issue.Type}]{issue.Message ?? message}"); }); + _ec.Setup(x => x.CreateIssue(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) + .Returns((IssueType type, string message, IssueMetadata metadata, bool writeToLog) => + { + var result = new Issue() + { + Type = type, + Message = message, + Category = metadata?.Category, + IsInfrastructureIssue = metadata?.IsInfrastructureIssue ?? false + }; + result.Data.AddRangeIfRangeNotNull(metadata?.Data); + return result; + }); + + _ec.Setup(x => x.AddIssue(It.IsAny())).Callback((IReadOnlyIssue issue) => { _hc.GetTrace().Info($"[{issue.Type}]{issue.Message}"); }); _hc.SetSingleton(_actionManager.Object); _hc.SetSingleton(_handlerFactory.Object); diff --git a/src/Test/L0/Worker/CreateStepSummaryCommandL0.cs b/src/Test/L0/Worker/CreateStepSummaryCommandL0.cs index 67631ed4bd7..69b2cbdda62 100644 --- a/src/Test/L0/Worker/CreateStepSummaryCommandL0.cs +++ b/src/Test/L0/Worker/CreateStepSummaryCommandL0.cs @@ -11,6 +11,7 @@ using DTWebApi = GitHub.DistributedTask.WebApi; using GitHub.DistributedTask.WebApi; using Pipelines = GitHub.DistributedTask.Pipelines; +using GitHub.Services.Common; namespace GitHub.Runner.Common.Tests.Worker { @@ -19,7 +20,7 @@ public sealed class CreateStepSummaryCommandL0 private Mock _executionContext; private Mock _jobServerQueue; private ExecutionContext _jobExecutionContext; - private List> _issues; + private List _issues; private Variables _variables; private string _rootDirectory; private CreateStepSummaryCommand _createStepCommand; @@ -186,7 +187,7 @@ private TestHostContext Setup([CallerMemberName] string name = "") { var hostContext = new TestHostContext(this, name); - _issues = new List>(); + _issues = new List(); // Setup a job request TaskOrchestrationPlanReference plan = new(); @@ -247,13 +248,26 @@ private TestHostContext Setup([CallerMemberName] string name = "") WriteDebug = true, Variables = _variables, }); - _executionContext.Setup(x => x.AddIssue(It.IsAny(), It.IsAny())) - .Callback((DTWebApi.Issue issue, string logMessage) => + _executionContext.Setup(x => x.AddIssue(It.IsAny())) + .Callback((DTWebApi.IReadOnlyIssue issue) => { - _issues.Add(new Tuple(issue, logMessage)); - var message = !string.IsNullOrEmpty(logMessage) ? logMessage : issue.Message; - _trace.Info($"Issue '{issue.Type}': {message}"); + _issues.Add(issue); + _trace.Info($"Issue '{issue.Type}': {issue.Message}"); }); + _executionContext.Setup(x => x.CreateIssue(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) + .Returns((IssueType type, string message, IssueMetadata metadata, bool writeToLog) => + { + var result = new Issue() + { + Type = type, + Message = message, + Category = metadata?.Category, + IsInfrastructureIssue = metadata?.IsInfrastructureIssue ?? false + }; + result.Data.AddRangeIfRangeNotNull(metadata?.Data); + return result; + }); + _executionContext.Setup(x => x.Write(It.IsAny(), It.IsAny())) .Callback((string tag, string message) => { diff --git a/src/Test/L0/Worker/ExecutionContextL0.cs b/src/Test/L0/Worker/ExecutionContextL0.cs index 3aef7fb58ff..91adbfb182a 100644 --- a/src/Test/L0/Worker/ExecutionContextL0.cs +++ b/src/Test/L0/Worker/ExecutionContextL0.cs @@ -52,36 +52,36 @@ public void AddIssue_CountWarningsErrors() // Act. ec.InitializeJob(jobRequest, CancellationToken.None); - ec.AddIssue(new Issue() { Type = IssueType.Error, Message = "error" }); - ec.AddIssue(new Issue() { Type = IssueType.Error, Message = "error" }); - ec.AddIssue(new Issue() { Type = IssueType.Error, Message = "error" }); - ec.AddIssue(new Issue() { Type = IssueType.Error, Message = "error" }); - ec.AddIssue(new Issue() { Type = IssueType.Error, Message = "error" }); - ec.AddIssue(new Issue() { Type = IssueType.Error, Message = "error" }); - ec.AddIssue(new Issue() { Type = IssueType.Error, Message = "error" }); - ec.AddIssue(new Issue() { Type = IssueType.Error, Message = "error" }); - ec.AddIssue(new Issue() { Type = IssueType.Error, Message = "error" }); - ec.AddIssue(new Issue() { Type = IssueType.Error, Message = "error" }); - ec.AddIssue(new Issue() { Type = IssueType.Error, Message = "error" }); - ec.AddIssue(new Issue() { Type = IssueType.Error, Message = "error" }); - ec.AddIssue(new Issue() { Type = IssueType.Error, Message = "error" }); - ec.AddIssue(new Issue() { Type = IssueType.Error, Message = "error" }); - ec.AddIssue(new Issue() { Type = IssueType.Error, Message = "error" }); - - ec.AddIssue(new Issue() { Type = IssueType.Warning, Message = "warning" }); - ec.AddIssue(new Issue() { Type = IssueType.Warning, Message = "warning" }); - ec.AddIssue(new Issue() { Type = IssueType.Warning, Message = "warning" }); - ec.AddIssue(new Issue() { Type = IssueType.Warning, Message = "warning" }); - ec.AddIssue(new Issue() { Type = IssueType.Warning, Message = "warning" }); - ec.AddIssue(new Issue() { Type = IssueType.Warning, Message = "warning" }); - ec.AddIssue(new Issue() { Type = IssueType.Warning, Message = "warning" }); - ec.AddIssue(new Issue() { Type = IssueType.Warning, Message = "warning" }); - ec.AddIssue(new Issue() { Type = IssueType.Warning, Message = "warning" }); - ec.AddIssue(new Issue() { Type = IssueType.Warning, Message = "warning" }); - ec.AddIssue(new Issue() { Type = IssueType.Warning, Message = "warning" }); - ec.AddIssue(new Issue() { Type = IssueType.Warning, Message = "warning" }); - ec.AddIssue(new Issue() { Type = IssueType.Warning, Message = "warning" }); - ec.AddIssue(new Issue() { Type = IssueType.Warning, Message = "warning" }); + ec.AddIssue(ec.CreateIssue(IssueType.Error, "error", null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Error, "error", null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Error, "error", null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Error, "error", null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Error, "error", null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Error, "error", null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Error, "error", null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Error, "error", null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Error, "error", null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Error, "error", null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Error, "error", null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Error, "error", null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Error, "error", null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Error, "error", null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Error, "error", null, true)); + + ec.AddIssue(ec.CreateIssue(IssueType.Warning, "warning", null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Warning, "warning", null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Warning, "warning", null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Warning, "warning", null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Warning, "warning", null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Warning, "warning", null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Warning, "warning", null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Warning, "warning", null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Warning, "warning", null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Warning, "warning", null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Warning, "warning", null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Warning, "warning", null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Warning, "warning", null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Warning, "warning", null, true)); ec.Complete(); @@ -190,9 +190,9 @@ public void AddIssue_TrimMessageSize() bigMessage += "a"; } - ec.AddIssue(new Issue() { Type = IssueType.Error, Message = bigMessage }); - ec.AddIssue(new Issue() { Type = IssueType.Warning, Message = bigMessage }); - ec.AddIssue(new Issue() { Type = IssueType.Notice, Message = bigMessage }); + ec.AddIssue(ec.CreateIssue(IssueType.Error, bigMessage, null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Warning, bigMessage, null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Notice, bigMessage, null, true)); ec.Complete(); @@ -242,9 +242,9 @@ public void AddIssue_AddStepAndLineNumberInformation() var embeddedStep = ec.CreateChild(Guid.NewGuid(), "action_1_pre", "action_1_pre", null, null, ActionRunStage.Main, isEmbedded: true); embeddedStep.Start(); - embeddedStep.AddIssue(new Issue() { Type = IssueType.Error, Message = "error annotation that should have step and line number information" }); - embeddedStep.AddIssue(new Issue() { Type = IssueType.Warning, Message = "warning annotation that should have step and line number information" }); - embeddedStep.AddIssue(new Issue() { Type = IssueType.Notice, Message = "notice annotation that should have step and line number information" }); + embeddedStep.AddIssue(embeddedStep.CreateIssue(IssueType.Error, "error annotation that should have step and line number information", null, true)); + embeddedStep.AddIssue(embeddedStep.CreateIssue(IssueType.Warning, "warning annotation that should have step and line number information", null, true)); + embeddedStep.AddIssue(embeddedStep.CreateIssue(IssueType.Notice, "notice annotation that should have step and line number information", null, true)); jobServerQueue.Verify(x => x.QueueTimelineRecordUpdate(It.IsAny(), It.Is(t => t.Issues.Where(i => i.Data.ContainsKey("stepNumber") && i.Data.ContainsKey("logFileLineNumber") && i.Type == IssueType.Error).Count() == 1)), Times.AtLeastOnce); jobServerQueue.Verify(x => x.QueueTimelineRecordUpdate(It.IsAny(), It.Is(t => t.Issues.Where(i => i.Data.ContainsKey("stepNumber") && i.Data.ContainsKey("logFileLineNumber") && i.Type == IssueType.Warning).Count() == 1)), Times.AtLeastOnce); @@ -626,12 +626,12 @@ public void PublishStepTelemetry_RegularStep() ec.StepTelemetry.StepId = Guid.NewGuid(); ec.StepTelemetry.Stage = "main"; - ec.AddIssue(new Issue() { Type = IssueType.Error, Message = "error" }); - ec.AddIssue(new Issue() { Type = IssueType.Warning, Message = "warning" }); - ec.AddIssue(new Issue() { Type = IssueType.Notice, Message = "notice" }); - ec.AddIssue(new Issue() { Type = IssueType.Error, Message = "error" }); - ec.AddIssue(new Issue() { Type = IssueType.Warning, Message = "warning" }); - ec.AddIssue(new Issue() { Type = IssueType.Notice, Message = "notice" }); + ec.AddIssue(ec.CreateIssue(IssueType.Error, "error", null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Warning, "warning", null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Notice, "notice", null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Error, "error", null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Warning, "warning", null, true)); + ec.AddIssue(ec.CreateIssue(IssueType.Notice, "notice", null, true)); ec.Complete(); @@ -692,9 +692,9 @@ public void PublishStepTelemetry_EmbeddedStep() embeddedStep.StepTelemetry.Action = "actions/checkout"; embeddedStep.StepTelemetry.Ref = "v2"; - embeddedStep.AddIssue(new Issue() { Type = IssueType.Error, Message = "error" }); - embeddedStep.AddIssue(new Issue() { Type = IssueType.Warning, Message = "warning" }); - embeddedStep.AddIssue(new Issue() { Type = IssueType.Notice, Message = "notice" }); + embeddedStep.AddIssue(ec.CreateIssue(IssueType.Error, "error", null, true)); + embeddedStep.AddIssue(ec.CreateIssue(IssueType.Warning, "warning", null, true)); + embeddedStep.AddIssue(ec.CreateIssue(IssueType.Notice, "notice", null, true)); embeddedStep.PublishStepTelemetry(); @@ -870,7 +870,7 @@ public void ActionVariables_AddedToVarsContext() inputVarsContext["VARIABLE_2"] = new StringContextData("value2"); jobRequest.ContextData["vars"] = inputVarsContext; - // Arrange: Setup the paging logger. + // Arrange: Setup the paging logger. var pagingLogger1 = new Mock(); var jobServerQueue = new Mock(); hc.EnqueueInstance(pagingLogger1.Object); @@ -884,7 +884,7 @@ public void ActionVariables_AddedToVarsContext() var expected = new DictionaryContextData(); expected["VARIABLE_1"] = new StringContextData("value1"); expected["VARIABLE_2"] = new StringContextData("value1"); - + Assert.True(ExpressionValuesAssertEqual(expected, jobContext.ExpressionValues["vars"] as DictionaryContextData)); } } @@ -915,7 +915,7 @@ public void ActionVariables_DebugUsingVars() inputVarsContext[Constants.Variables.Actions.RunnerDebug] = new StringContextData("true"); jobRequest.ContextData["vars"] = inputVarsContext; - // Arrange: Setup the paging logger. + // Arrange: Setup the paging logger. var pagingLogger1 = new Mock(); var jobServerQueue = new Mock(); hc.EnqueueInstance(pagingLogger1.Object); @@ -926,7 +926,7 @@ public void ActionVariables_DebugUsingVars() jobContext.InitializeJob(jobRequest, CancellationToken.None); - + Assert.Equal("true", jobContext.Global.Variables.Get(Constants.Variables.Actions.StepDebug)); Assert.Equal("true", jobContext.Global.Variables.Get(Constants.Variables.Actions.RunnerDebug)); } @@ -961,7 +961,7 @@ public void ActionVariables_SecretsPrecedenceForDebugUsingVars() jobRequest.Variables[Constants.Variables.Actions.StepDebug] = "false"; jobRequest.Variables[Constants.Variables.Actions.RunnerDebug] = "false"; - // Arrange: Setup the paging logger. + // Arrange: Setup the paging logger. var pagingLogger1 = new Mock(); var jobServerQueue = new Mock(); hc.EnqueueInstance(pagingLogger1.Object); @@ -972,7 +972,7 @@ public void ActionVariables_SecretsPrecedenceForDebugUsingVars() jobContext.InitializeJob(jobRequest, CancellationToken.None); - + Assert.Equal("false", jobContext.Global.Variables.Get(Constants.Variables.Actions.StepDebug)); Assert.Equal("false", jobContext.Global.Variables.Get(Constants.Variables.Actions.RunnerDebug)); } diff --git a/src/Test/L0/Worker/OutputManagerL0.cs b/src/Test/L0/Worker/OutputManagerL0.cs index e0364e9f0fc..e511bd73f2a 100644 --- a/src/Test/L0/Worker/OutputManagerL0.cs +++ b/src/Test/L0/Worker/OutputManagerL0.cs @@ -9,6 +9,7 @@ using GitHub.Runner.Worker; using GitHub.Runner.Worker.Container; using GitHub.Runner.Worker.Handlers; +using GitHub.Services.Common; using Moq; using Xunit; using DTWebApi = GitHub.DistributedTask.WebApi; @@ -21,7 +22,7 @@ public sealed class OutputManagerL0 private Mock _commandManager; private Variables _variables; private OnMatcherChanged _onMatcherChanged; - private List> _issues; + private List _issues; private List _messages; private List _commands; private OutputManager _outputManager; @@ -82,10 +83,10 @@ public void AddMatcher_Clobber() Process("ERROR: message 4"); Process("NOT GOOD: message 5"); Assert.Equal(4, _issues.Count); - Assert.Equal("message 1", _issues[0].Item1.Message); - Assert.Equal("message 2", _issues[1].Item1.Message); - Assert.Equal("message 3", _issues[2].Item1.Message); - Assert.Equal("message 5", _issues[3].Item1.Message); + Assert.Equal("message 1", _issues[0].Message); + Assert.Equal("message 2", _issues[1].Message); + Assert.Equal("message 3", _issues[2].Message); + Assert.Equal("message 5", _issues[3].Message); Assert.Equal(0, _commands.Count); Assert.Equal(1, _messages.Count); Assert.Equal("ERROR: message 4", _messages[0]); @@ -148,11 +149,11 @@ public void AddMatcher_Prepend() Process("ERROR: message 4"); Process("NOT GOOD: message 5"); Assert.Equal(5, _issues.Count); - Assert.Equal("message 1", _issues[0].Item1.Message); - Assert.Equal("message 2", _issues[1].Item1.Message); - Assert.Equal("message 3", _issues[2].Item1.Message); - Assert.Equal("message 4", _issues[3].Item1.Message); - Assert.Equal("message 5", _issues[4].Item1.Message); + Assert.Equal("message 1", _issues[0].Message); + Assert.Equal("message 2", _issues[1].Message); + Assert.Equal("message 3", _issues[2].Message); + Assert.Equal("message 4", _issues[3].Message); + Assert.Equal("message 5", _issues[4].Message); Assert.Equal(0, _commands.Count); Assert.Equal(0, _messages.Count); } @@ -188,10 +189,10 @@ public void MatcherCode() Process("BAD: real bad"); Process(": not working"); Assert.Equal(2, _issues.Count); - Assert.Equal("real bad", _issues[0].Item1.Message); - Assert.Equal("BAD", _issues[0].Item1.Data["code"]); - Assert.Equal("not working", _issues[1].Item1.Message); - Assert.False(_issues[1].Item1.Data.ContainsKey("code")); + Assert.Equal("real bad", _issues[0].Message); + Assert.Equal("BAD", _issues[0]["code"]); + Assert.Equal("not working", _issues[1].Message); + Assert.Null(_issues[1]["code"]); Assert.Equal(0, _commands.Count); Assert.Equal(0, _messages.Count); } @@ -238,11 +239,11 @@ public void DoesNotResetMatchingMatcher() Process("Error: real bad"); Process("regular message 2"); Assert.Equal(5, _issues.Count); - Assert.Equal("it broke", _issues[0].Item1.Message); - Assert.Equal("oh no", _issues[1].Item1.Message); - Assert.Equal("not good", _issues[2].Item1.Message); - Assert.Equal("it broke again", _issues[3].Item1.Message); - Assert.Equal("real bad", _issues[4].Item1.Message); + Assert.Equal("it broke", _issues[0].Message); + Assert.Equal("oh no", _issues[1].Message); + Assert.Equal("not good", _issues[2].Message); + Assert.Equal("it broke again", _issues[3].Message); + Assert.Equal("real bad", _issues[4].Message); Assert.Equal(0, _commands.Count); Assert.Equal(4, _messages.Count); Assert.Equal("Start: hello", _messages[0]); @@ -293,8 +294,8 @@ public void InitialMatchers() Process("ERROR: it is broken"); Process("NOT GOOD: that did not work"); Assert.Equal(2, _issues.Count); - Assert.Equal("it is broken", _issues[0].Item1.Message); - Assert.Equal("that did not work", _issues[1].Item1.Message); + Assert.Equal("it is broken", _issues[0].Message); + Assert.Equal("that did not work", _issues[1].Message); Assert.Equal(0, _commands.Count); Assert.Equal(0, _messages.Count); } @@ -332,15 +333,15 @@ public void MatcherLineColumn() Process("(12,thirty-four): it is broken"); Process("(twelve,34): not working"); Assert.Equal(3, _issues.Count); - Assert.Equal("real bad", _issues[0].Item1.Message); - Assert.Equal("12", _issues[0].Item1.Data["line"]); - Assert.Equal("34", _issues[0].Item1.Data["col"]); - Assert.Equal("it is broken", _issues[1].Item1.Message); - Assert.Equal("12", _issues[1].Item1.Data["line"]); - Assert.False(_issues[1].Item1.Data.ContainsKey("col")); - Assert.Equal("not working", _issues[2].Item1.Message); - Assert.False(_issues[2].Item1.Data.ContainsKey("line")); - Assert.Equal("34", _issues[2].Item1.Data["col"]); + Assert.Equal("real bad", _issues[0].Message); + Assert.Equal("12", _issues[0]["line"]); + Assert.Equal("34", _issues[0]["col"]); + Assert.Equal("it is broken", _issues[1].Message); + Assert.Equal("12", _issues[1]["line"]); + Assert.Null(_issues[1]["col"]); + Assert.Equal("not working", _issues[2].Message); + Assert.Null(_issues[2]["line"]); + Assert.Equal("34", _issues[2]["col"]); Assert.Equal(0, _commands.Count); Assert.Equal(2, _messages.Count); Assert.Equal("##[debug]Unable to parse column number 'thirty-four'", _messages[0]); @@ -373,8 +374,8 @@ public void MatcherDoesNotReceiveCommand() Process("this line is a command too ##[some-command]even though it contains ERROR: not working again"); Process("##[not-command]this line is an ERROR: it is broken again"); Assert.Equal(2, _issues.Count); - Assert.Equal("it is broken", _issues[0].Item1.Message); - Assert.Equal("it is broken again", _issues[1].Item1.Message); + Assert.Equal("it is broken", _issues[0].Message); + Assert.Equal("it is broken again", _issues[1].Message); Assert.Equal(2, _commands.Count); Assert.Equal("##[some-command]this line is a command even though it contains ERROR: not working", _commands[0]); Assert.Equal("this line is a command too ##[some-command]even though it contains ERROR: not working again", _commands[1]); @@ -404,8 +405,7 @@ public void MatcherRemoveColorCodes() }); Process("the error: \033[31mred, \033[1;31mbright red, \033[mreset"); Assert.Equal(1, _issues.Count); - Assert.Equal("red, bright red, reset", _issues[0].Item1.Message); - Assert.Equal("the error: red, bright red, reset", _issues[0].Item2); + Assert.Equal("red, bright red, reset", _issues[0].Message); Assert.Equal(0, _commands.Count); Assert.Equal(0, _messages.Count); } @@ -455,9 +455,9 @@ public void RemoveMatcher() Process("ERROR: message 3"); Process("NOT GOOD: message 4"); Assert.Equal(3, _issues.Count); - Assert.Equal("message 1", _issues[0].Item1.Message); - Assert.Equal("message 2", _issues[1].Item1.Message); - Assert.Equal("message 4", _issues[2].Item1.Message); + Assert.Equal("message 1", _issues[0].Message); + Assert.Equal("message 2", _issues[1].Message); + Assert.Equal("message 4", _issues[2].Message); Assert.Equal(0, _commands.Count); Assert.Equal(1, _messages.Count); Assert.Equal("ERROR: message 3", _messages[0]); @@ -517,8 +517,8 @@ public void ResetsOtherMatchers() Process("Matches both line 1: hello again"); Process("oh no, another error"); Assert.Equal(2, _issues.Count); - Assert.Equal("it broke", _issues[0].Item1.Message); - Assert.Equal("oh no, another error", _issues[1].Item1.Message); + Assert.Equal("it broke", _issues[0].Message); + Assert.Equal("oh no, another error", _issues[1].Message); Assert.Equal(0, _commands.Count); Assert.Equal(4, _messages.Count); Assert.Equal("Matches both line 1: hello", _messages[0]); @@ -573,14 +573,14 @@ public void MatcherSeverity() Process(": not working"); Process("ERROR! uh oh"); Assert.Equal(4, _issues.Count); - Assert.Equal("real bad", _issues[0].Item1.Message); - Assert.Equal(DTWebApi.IssueType.Error, _issues[0].Item1.Type); - Assert.Equal("not great", _issues[1].Item1.Message); - Assert.Equal(DTWebApi.IssueType.Warning, _issues[1].Item1.Type); - Assert.Equal("not working", _issues[2].Item1.Message); - Assert.Equal(DTWebApi.IssueType.Error, _issues[2].Item1.Type); - Assert.Equal("uh oh", _issues[3].Item1.Message); - Assert.Equal(DTWebApi.IssueType.Error, _issues[3].Item1.Type); + Assert.Equal("real bad", _issues[0].Message); + Assert.Equal(DTWebApi.IssueType.Error, _issues[0].Type); + Assert.Equal("not great", _issues[1].Message); + Assert.Equal(DTWebApi.IssueType.Warning, _issues[1].Type); + Assert.Equal("not working", _issues[2].Message); + Assert.Equal(DTWebApi.IssueType.Error, _issues[2].Type); + Assert.Equal("uh oh", _issues[3].Message); + Assert.Equal(DTWebApi.IssueType.Error, _issues[3].Type); Assert.Equal(0, _commands.Count); Assert.Equal(2, _messages.Count); Assert.StartsWith("##[debug]Skipped", _messages[0]); @@ -633,9 +633,9 @@ public void MatcherTimeout() Process("jane.doe@contoso.com"); Process("ERR: this error"); Assert.Equal(3, _issues.Count); - Assert.Equal("john.doe@contoso.com", _issues[0].Item1.Message); - Assert.Contains("Removing issue matcher 'email'", _issues[1].Item1.Message); - Assert.Equal("this error", _issues[2].Item1.Message); + Assert.Equal("john.doe@contoso.com", _issues[0].Message); + Assert.Contains("Removing issue matcher 'email'", _issues[1].Message); + Assert.Equal("this error", _issues[2].Message); Assert.Equal(0, _commands.Count); Assert.Equal(2, _messages.Where(x => x.StartsWith("##[debug]Timeout processing issue matcher")).Count()); Assert.Equal(1, _messages.Where(x => x.Equals("t@t.t.t.t.t.t.t.t.t.t.t.t.t.t.t.t.t.t.t.t.t.t.t.t.t.t.t.t.t.t.t.t.t.t.t.t.t.t.t.t.t.t.t.t.t.t.t.c%20")).Count()); @@ -725,32 +725,32 @@ public async void MatcherFile() Assert.Equal(9, _issues.Count); - Assert.Equal("some error 1", _issues[0].Item1.Message); - Assert.False(_issues[0].Item1.Data.ContainsKey("file")); + Assert.Equal("some error 1", _issues[0].Message); + Assert.Null(_issues[0]["file"]); - Assert.Equal("some error 2", _issues[1].Item1.Message); - Assert.Equal(file_workflowRepository.Substring(workflowRepository.Length + 1).Replace(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar), _issues[1].Item1.Data["file"]); + Assert.Equal("some error 2", _issues[1].Message); + Assert.Equal(file_workflowRepository.Substring(workflowRepository.Length + 1).Replace(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar), _issues[1]["file"]); - Assert.Equal("some error 3", _issues[2].Item1.Message); - Assert.Equal(file_workflowRepository.Substring(workflowRepository.Length + 1).Replace(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar), _issues[2].Item1.Data["file"]); + Assert.Equal("some error 3", _issues[2].Message); + Assert.Equal(file_workflowRepository.Substring(workflowRepository.Length + 1).Replace(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar), _issues[2]["file"]); - Assert.Equal("some error 4", _issues[3].Item1.Message); - Assert.Equal(file_workflowRepository_nestedDirectory.Substring(workflowRepository.Length + 1).Replace(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar), _issues[3].Item1.Data["file"]); + Assert.Equal("some error 4", _issues[3].Message); + Assert.Equal(file_workflowRepository_nestedDirectory.Substring(workflowRepository.Length + 1).Replace(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar), _issues[3]["file"]); - Assert.Equal("some error 5", _issues[4].Item1.Message); - Assert.False(_issues[4].Item1.Data.ContainsKey("file")); + Assert.Equal("some error 5", _issues[4].Message); + Assert.Null(_issues[4]["file"]); - Assert.Equal("some error 6", _issues[5].Item1.Message); - Assert.False(_issues[5].Item1.Data.ContainsKey("file")); + Assert.Equal("some error 6", _issues[5].Message); + Assert.Null(_issues[5]["file"]); - Assert.Equal("some error 7", _issues[6].Item1.Message); - Assert.False(_issues[6].Item1.Data.ContainsKey("file")); + Assert.Equal("some error 7", _issues[6].Message); + Assert.Null(_issues[6]["file"]); - Assert.Equal("some error 8", _issues[7].Item1.Message); - Assert.Equal(file_nestedWorkflowRepository.Substring(nestedWorkflowRepository.Length + 1).Replace(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar), _issues[7].Item1.Data["file"]); + Assert.Equal("some error 8", _issues[7].Message); + Assert.Equal(file_nestedWorkflowRepository.Substring(nestedWorkflowRepository.Length + 1).Replace(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar), _issues[7]["file"]); - Assert.Equal("some error 9", _issues[8].Item1.Message); - Assert.Equal(file_workflowRepositoryUsingSsh.Substring(workflowRepositoryUsingSsh.Length + 1).Replace(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar), _issues[8].Item1.Data["file"]); + Assert.Equal("some error 9", _issues[8].Message); + Assert.Equal(file_workflowRepositoryUsingSsh.Substring(workflowRepositoryUsingSsh.Length + 1).Replace(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar), _issues[8]["file"]); } Environment.SetEnvironmentVariable("RUNNER_TEST_GET_REPOSITORY_PATH_FAILSAFE", ""); @@ -810,11 +810,11 @@ public async void MatcherFile_JobContainer() Assert.Equal(2, _issues.Count); - Assert.Equal("some error 1", _issues[0].Item1.Message); - Assert.Equal("some-file.txt", _issues[0].Item1.Data["file"]); + Assert.Equal("some error 1", _issues[0].Message); + Assert.Equal("some-file.txt", _issues[0]["file"]); - Assert.Equal("some error 2", _issues[1].Item1.Message); - Assert.Equal("some-file.txt", _issues[1].Item1.Data["file"]); + Assert.Equal("some error 2", _issues[1].Message); + Assert.Equal("some-file.txt", _issues[1]["file"]); } } @@ -871,11 +871,11 @@ public async void MatcherFile_StepContainer() Assert.Equal(2, _issues.Count); - Assert.Equal("some error 1", _issues[0].Item1.Message); - Assert.Equal("some-file.txt", _issues[0].Item1.Data["file"]); + Assert.Equal("some error 1", _issues[0].Message); + Assert.Equal("some-file.txt", _issues[0]["file"]); - Assert.Equal("some error 2", _issues[1].Item1.Message); - Assert.Equal("some-file.txt", _issues[1].Item1.Data["file"]); + Assert.Equal("some error 2", _issues[1].Message); + Assert.Equal("some-file.txt", _issues[1]["file"]); } } #endif @@ -929,8 +929,8 @@ public async void MatcherFromPath() // Process Process("some-directory/some-file.txt: some error [workflow-repo/some-project/some-project.proj]"); Assert.Equal(1, _issues.Count); - Assert.Equal("some error", _issues[0].Item1.Message); - Assert.Equal("some-project/some-directory/some-file.txt", _issues[0].Item1.Data["file"]); + Assert.Equal("some error", _issues[0].Message); + Assert.Equal("some-project/some-directory/some-file.txt", _issues[0]["file"]); Assert.Equal(0, _commands.Count); Assert.Equal(0, _messages.Count); } @@ -958,7 +958,7 @@ private TestHostContext Setup( matchers?.Validate(); _onMatcherChanged = null; - _issues = new List>(); + _issues = new List(); _messages = new List(); _commands = new List(); @@ -983,10 +983,24 @@ private TestHostContext Setup( { _onMatcherChanged = handler; }); - _executionContext.Setup(x => x.AddIssue(It.IsAny(), It.IsAny())) - .Callback((DTWebApi.Issue issue, string logMessage) => + _executionContext.Setup(x => x.CreateIssue(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) + .Returns((DTWebApi.IssueType type, string message, DTWebApi.IssueMetadata metadata, bool writeToLog) => { - _issues.Add(new Tuple(issue, logMessage)); + var result = new DTWebApi.Issue() + { + Type = type, + Message = message, + Category = metadata?.Category, + IsInfrastructureIssue = metadata?.IsInfrastructureIssue ?? false + }; + result.Data.AddRangeIfRangeNotNull(metadata?.Data); + return result; + }); + + _executionContext.Setup(x => x.AddIssue(It.IsAny())) + .Callback((DTWebApi.IReadOnlyIssue issue) => + { + _issues.Add(issue); }); _executionContext.Setup(x => x.Write(It.IsAny(), It.IsAny())) .Callback((string tag, string message) => diff --git a/src/Test/L0/Worker/SaveStateFileCommandL0.cs b/src/Test/L0/Worker/SaveStateFileCommandL0.cs index 45296c70978..01503f2b2ec 100644 --- a/src/Test/L0/Worker/SaveStateFileCommandL0.cs +++ b/src/Test/L0/Worker/SaveStateFileCommandL0.cs @@ -21,7 +21,7 @@ namespace GitHub.Runner.Common.Tests.Worker public sealed class SaveStateFileCommandL0 { private Mock _executionContext; - private List> _issues; + private List _issues; private string _rootDirectory; private SaveStateFileCommand _saveStateFileCommand; private Dictionary _intraActionState; @@ -390,7 +390,7 @@ private void WriteContent( private TestHostContext Setup([CallerMemberName] string name = "") { - _issues = new List>(); + _issues = new List(); _intraActionState = new Dictionary(); var hostContext = new TestHostContext(this, name); @@ -413,12 +413,11 @@ private TestHostContext Setup([CallerMemberName] string name = "") EnvironmentVariables = new Dictionary(VarUtil.EnvironmentVariableKeyComparer), WriteDebug = true, }); - _executionContext.Setup(x => x.AddIssue(It.IsAny(), It.IsAny())) - .Callback((DTWebApi.Issue issue, string logMessage) => + _executionContext.Setup(x => x.AddIssue(It.IsAny())) + .Callback((DTWebApi.IReadOnlyIssue issue) => { - _issues.Add(new Tuple(issue, logMessage)); - var message = !string.IsNullOrEmpty(logMessage) ? logMessage : issue.Message; - _trace.Info($"Issue '{issue.Type}': {message}"); + _issues.Add(issue); + _trace.Info($"Issue '{issue.Type}': {issue.Message}"); }); _executionContext.Setup(x => x.Write(It.IsAny(), It.IsAny())) .Callback((string tag, string message) => diff --git a/src/Test/L0/Worker/SetEnvFileCommandL0.cs b/src/Test/L0/Worker/SetEnvFileCommandL0.cs index 41f8499d582..a49b896556e 100644 --- a/src/Test/L0/Worker/SetEnvFileCommandL0.cs +++ b/src/Test/L0/Worker/SetEnvFileCommandL0.cs @@ -1,17 +1,11 @@ using System; using System.Collections.Generic; -using System.Globalization; using System.IO; -using System.Linq; using System.Text; -using System.Threading; -using System.Threading.Tasks; using System.Runtime.CompilerServices; using GitHub.Runner.Common.Util; using GitHub.Runner.Sdk; using GitHub.Runner.Worker; -using GitHub.Runner.Worker.Container; -using GitHub.Runner.Worker.Handlers; using Moq; using Xunit; using DTWebApi = GitHub.DistributedTask.WebApi; @@ -21,7 +15,7 @@ namespace GitHub.Runner.Common.Tests.Worker public sealed class SetEnvFileCommandL0 { private Mock _executionContext; - private List> _issues; + private List _issues; private string _rootDirectory; private SetEnvFileCommand _setEnvFileCommand; private ITraceWriter _trace; @@ -389,7 +383,7 @@ private void WriteContent( private TestHostContext Setup([CallerMemberName] string name = "") { - _issues = new List>(); + _issues = new List(); var hostContext = new TestHostContext(this, name); @@ -411,12 +405,11 @@ private TestHostContext Setup([CallerMemberName] string name = "") EnvironmentVariables = new Dictionary(VarUtil.EnvironmentVariableKeyComparer), WriteDebug = true, }); - _executionContext.Setup(x => x.AddIssue(It.IsAny(), It.IsAny())) - .Callback((DTWebApi.Issue issue, string logMessage) => + _executionContext.Setup(x => x.AddIssue(It.IsAny())) + .Callback((DTWebApi.IReadOnlyIssue issue) => { - _issues.Add(new Tuple(issue, logMessage)); - var message = !string.IsNullOrEmpty(logMessage) ? logMessage : issue.Message; - _trace.Info($"Issue '{issue.Type}': {message}"); + _issues.Add(issue); + _trace.Info($"Issue '{issue.Type}': {issue.Message}"); }); _executionContext.Setup(x => x.Write(It.IsAny(), It.IsAny())) .Callback((string tag, string message) => diff --git a/src/Test/L0/Worker/SetOutputFileCommandL0.cs b/src/Test/L0/Worker/SetOutputFileCommandL0.cs index 8af9695db46..0a9fea55fed 100644 --- a/src/Test/L0/Worker/SetOutputFileCommandL0.cs +++ b/src/Test/L0/Worker/SetOutputFileCommandL0.cs @@ -1,17 +1,11 @@ using System; using System.Collections.Generic; -using System.Globalization; using System.IO; -using System.Linq; using System.Text; -using System.Threading; -using System.Threading.Tasks; using System.Runtime.CompilerServices; using GitHub.Runner.Common.Util; using GitHub.Runner.Sdk; using GitHub.Runner.Worker; -using GitHub.Runner.Worker.Container; -using GitHub.Runner.Worker.Handlers; using Moq; using Xunit; using DTWebApi = GitHub.DistributedTask.WebApi; @@ -21,7 +15,7 @@ namespace GitHub.Runner.Common.Tests.Worker public sealed class SetOutputFileCommandL0 { private Mock _executionContext; - private List> _issues; + private List _issues; private Dictionary _outputs; private string _rootDirectory; private SetOutputFileCommand _setOutputFileCommand; @@ -390,7 +384,7 @@ private void WriteContent( private TestHostContext Setup([CallerMemberName] string name = "") { - _issues = new List>(); + _issues = new List(); _outputs = new Dictionary(); var hostContext = new TestHostContext(this, name); @@ -413,12 +407,11 @@ private TestHostContext Setup([CallerMemberName] string name = "") EnvironmentVariables = new Dictionary(VarUtil.EnvironmentVariableKeyComparer), WriteDebug = true, }); - _executionContext.Setup(x => x.AddIssue(It.IsAny(), It.IsAny())) - .Callback((DTWebApi.Issue issue, string logMessage) => + _executionContext.Setup(x => x.AddIssue(It.IsAny())) + .Callback((DTWebApi.IReadOnlyIssue issue) => { - _issues.Add(new Tuple(issue, logMessage)); - var message = !string.IsNullOrEmpty(logMessage) ? logMessage : issue.Message; - _trace.Info($"Issue '{issue.Type}': {message}"); + _issues.Add(issue); + _trace.Info($"Issue '{issue.Type}': {issue.Message}"); }); _executionContext.Setup(x => x.Write(It.IsAny(), It.IsAny())) .Callback((string tag, string message) => From efc0a92cc73aa97b7c014d3f1184223401443ae7 Mon Sep 17 00:00:00 2001 From: John Wesley Walker III <81404201+jww3@users.noreply.github.com> Date: Tue, 17 Jan 2023 13:43:04 +0000 Subject: [PATCH 3/9] Let the `IssueMetadata` class be the mechanism for specifying log message overrides. --- src/Runner.Worker/ActionCommandManager.cs | 2 +- src/Runner.Worker/ExecutionContext.cs | 13 ++++-- src/Runner.Worker/Handlers/OutputManager.cs | 8 ++-- src/Runner.Worker/IssueMatcher.cs | 35 +++++++++++++--- src/Sdk/DTWebApi/WebApi/Issue.cs | 44 +++++---------------- src/Test/L0/Worker/OutputManagerL0.cs | 4 +- 6 files changed, 55 insertions(+), 51 deletions(-) diff --git a/src/Runner.Worker/ActionCommandManager.cs b/src/Runner.Worker/ActionCommandManager.cs index 42230588b7e..1691d0eb09d 100644 --- a/src/Runner.Worker/ActionCommandManager.cs +++ b/src/Runner.Worker/ActionCommandManager.cs @@ -648,7 +648,7 @@ public void ProcessCommand(IExecutionContext context, string inputLine, ActionCo var filteredDictionaryEntries = command.Properties .Where(kvp => !string.Equals(kvp.Key, keyToExclude, StringComparison.OrdinalIgnoreCase)); - var metadata = new IssueMetadata(issueCategory, false, filteredDictionaryEntries); + var metadata = new IssueMetadata(issueCategory, false, null, filteredDictionaryEntries); var issue = context.CreateIssue(this.Type, command.Data, metadata, true); context.AddIssue(issue); } diff --git a/src/Runner.Worker/ExecutionContext.cs b/src/Runner.Worker/ExecutionContext.cs index ec59b029d49..1f7c6e6206a 100644 --- a/src/Runner.Worker/ExecutionContext.cs +++ b/src/Runner.Worker/ExecutionContext.cs @@ -620,10 +620,15 @@ public IReadOnlyIssue CreateIssue(IssueType issueType, string rawMessage, IssueM if (!string.IsNullOrEmpty(wellKnownTag)) { - if (writeToLog && !string.IsNullOrEmpty(refinedMessage)) + if (writeToLog) { - long logLineNumber = Write(wellKnownTag, refinedMessage); - result.Data["logFileLineNumber"] = logLineNumber.ToString(); + //Note that ::Write() has it's own secret masking logic + string logText = metadata?.LogMessageOverride ?? result.Message; + if (!string.IsNullOrEmpty(logText)) + { + long logLineNumber = Write(wellKnownTag, logText); + result.Data["logFileLineNumber"] = logLineNumber.ToString(); + } } if (previousCountForIssueType.GetValueOrDefault(0) < _maxIssueCount) { @@ -1192,7 +1197,7 @@ public static void Error(this IExecutionContext context, string message) // Do not add a format string overload. See comment on ExecutionContext.Write(). public static void InfrastructureError(this IExecutionContext context, string message) { - var metadata = new IssueMetadata(null, true, Enumerable.Empty>()); + var metadata = new IssueMetadata(null, true, null, Enumerable.Empty>()); var issue = context.CreateIssue(IssueType.Error, message, metadata, true); context.AddIssue(issue); } diff --git a/src/Runner.Worker/Handlers/OutputManager.cs b/src/Runner.Worker/Handlers/OutputManager.cs index 3f5301e15d9..6ab611b5d87 100644 --- a/src/Runner.Worker/Handlers/OutputManager.cs +++ b/src/Runner.Worker/Handlers/OutputManager.cs @@ -98,7 +98,7 @@ public void OnDataReceived(object sender, ProcessDataReceivedEventArgs e) var matchers = _matchers; // Strip color codes - var stripped = line.Contains(_colorCodePrefix) ? _colorCodeRegex.Replace(line, string.Empty) : line; + var refinedLine = line.Contains(_colorCodePrefix) ? _colorCodeRegex.Replace(line, string.Empty) : line; foreach (var matcher in matchers) { @@ -108,7 +108,7 @@ public void OnDataReceived(object sender, ProcessDataReceivedEventArgs e) // Match try { - match = matcher.Match(stripped); + match = matcher.Match(refinedLine); break; } catch (RegexMatchTimeoutException ex) @@ -116,7 +116,7 @@ public void OnDataReceived(object sender, ProcessDataReceivedEventArgs e) if (attempt < _maxAttempts) { // Debug - _executionContext.Debug($"Timeout processing issue matcher '{matcher.Owner}' against line '{stripped}'. Exception: {ex}"); + _executionContext.Debug($"Timeout processing issue matcher '{matcher.Owner}' against line '{refinedLine}'. Exception: {ex}"); } else { @@ -324,7 +324,7 @@ private DTWebApi.IReadOnlyIssue ConvertToIssue(IssueMatch match) _executionContext.Debug($"Dropping file value '{match.File}' and fromPath value '{match.FromPath}'. Exception during validation: {ex}"); } - var metadata = new IssueMetadata(null, false, issueData); + var metadata = new IssueMetadata(null, false, match.SourceText, issueData); var issue = _executionContext.CreateIssue(issueType, match.Message, metadata, true); return issue; } diff --git a/src/Runner.Worker/IssueMatcher.cs b/src/Runner.Worker/IssueMatcher.cs index 0a6eb0fc4a4..a1e85fed5f8 100644 --- a/src/Runner.Worker/IssueMatcher.cs +++ b/src/Runner.Worker/IssueMatcher.cs @@ -8,6 +8,28 @@ namespace GitHub.Runner.Worker { public delegate void OnMatcherChanged(object sender, MatcherChangedEventArgs e); + + public sealed class IssueMetadata + { + public IssueMetadata(string key, string value) + : this(null, false, null, new []{ KeyValuePair.Create(key, value) }) + { + } + + public IssueMetadata(string category, bool infrastructureIssue, string logMessageOverride, IEnumerable> data) + { + this.Category = category; + this.IsInfrastructureIssue = infrastructureIssue; + this.LogMessageOverride = logMessageOverride; + this.Data = data; + } + + public readonly string Category; + public readonly bool IsInfrastructureIssue; + public readonly string LogMessageOverride; + public readonly IEnumerable> Data; + } + public sealed class MatcherChangedEventArgs : EventArgs { public MatcherChangedEventArgs(IssueMatcherConfig config) @@ -69,7 +91,7 @@ public IssueMatch Match(string line) if (regexMatch.Success) { - return new IssueMatch(null, pattern, regexMatch.Groups, DefaultSeverity); + return new IssueMatch(line, null, pattern, regexMatch.Groups, DefaultSeverity); } return null; @@ -110,13 +132,13 @@ public IssueMatch Match(string line) } // Return - return new IssueMatch(runningMatch, pattern, regexMatch.Groups, DefaultSeverity); + return new IssueMatch(line, runningMatch, pattern, regexMatch.Groups, DefaultSeverity); } // Not the last pattern else { // Store the match - _state[i] = new IssueMatch(runningMatch, pattern, regexMatch.Groups); + _state[i] = new IssueMatch(line, runningMatch, pattern, regexMatch.Groups); } } // Not matched @@ -184,8 +206,9 @@ public IssuePattern(IssuePatternConfig config, TimeSpan timeout) public sealed class IssueMatch { - public IssueMatch(IssueMatch runningMatch, IssuePattern pattern, GroupCollection groups, string defaultSeverity = null) + public IssueMatch(string sourceText, IssueMatch runningMatch, IssuePattern pattern, GroupCollection groups, string defaultSeverity = null) { + SourceText = sourceText; File = runningMatch?.File ?? GetValue(groups, pattern.File); Line = runningMatch?.Line ?? GetValue(groups, pattern.Line); Column = runningMatch?.Column ?? GetValue(groups, pattern.Column); @@ -200,6 +223,8 @@ public IssueMatch(IssueMatch runningMatch, IssuePattern pattern, GroupCollection } } + public string SourceText { get; } + public string File { get; } public string Line { get; } @@ -455,7 +480,7 @@ public void Validate( if (Loop && Message == null) { throw new ArgumentException($"The {_loopPropertyName} pattern must set '{_messagePropertyName}'"); - } + } var regex = new Regex(Pattern ?? string.Empty, RegexOptions); var groupCount = regex.GetGroupNumbers().Length; diff --git a/src/Sdk/DTWebApi/WebApi/Issue.cs b/src/Sdk/DTWebApi/WebApi/Issue.cs index d68815402ac..c9dbbd789e2 100644 --- a/src/Sdk/DTWebApi/WebApi/Issue.cs +++ b/src/Sdk/DTWebApi/WebApi/Issue.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.Runtime.Serialization; +using GitHub.Services.Common; namespace GitHub.DistributedTask.WebApi { @@ -14,48 +15,25 @@ public interface IReadOnlyIssue string this[string key] { get; } } - public class IssueMetadata - { - - public IssueMetadata(string key, string value) - : this(null, false, new []{ KeyValuePair.Create(key, value) }) - { - } - - public IssueMetadata(string category, bool infrastructureIssue, IEnumerable> data) - { - this.Category = category; - this.IsInfrastructureIssue = infrastructureIssue; - this.Data = data; - } - - - public readonly string Category; - public readonly bool IsInfrastructureIssue; - public readonly IEnumerable> Data; - } - [DataContract] public class Issue : IReadOnlyIssue { public Issue() + : this(null) { } private Issue(Issue original) { - this.Type = original.Type; - this.Category = original.Category; - this.Message = original.Message; - this.IsInfrastructureIssue = original.IsInfrastructureIssue; - - if (original.m_data != null) + m_data = new Dictionary(StringComparer.OrdinalIgnoreCase); + if (original != null) { - foreach (var item in original.m_data) - { - this.Data.Add(item); - } + this.Type = original.Type; + this.Category = original.Category; + this.Message = original.Message; + this.IsInfrastructureIssue = original.IsInfrastructureIssue; + this.Data.AddRange(original.Data); } } @@ -91,10 +69,6 @@ public IDictionary Data { get { - if (m_data == null) - { - m_data = new Dictionary(StringComparer.OrdinalIgnoreCase); - } return m_data; } } diff --git a/src/Test/L0/Worker/OutputManagerL0.cs b/src/Test/L0/Worker/OutputManagerL0.cs index e511bd73f2a..09d98d96557 100644 --- a/src/Test/L0/Worker/OutputManagerL0.cs +++ b/src/Test/L0/Worker/OutputManagerL0.cs @@ -983,8 +983,8 @@ private TestHostContext Setup( { _onMatcherChanged = handler; }); - _executionContext.Setup(x => x.CreateIssue(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) - .Returns((DTWebApi.IssueType type, string message, DTWebApi.IssueMetadata metadata, bool writeToLog) => + _executionContext.Setup(x => x.CreateIssue(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) + .Returns((DTWebApi.IssueType type, string message, IssueMetadata metadata, bool writeToLog) => { var result = new DTWebApi.Issue() { From 4eb9adc958b0b2fc513306cce30fea6779a228bf Mon Sep 17 00:00:00 2001 From: John Wesley Walker III <81404201+jww3@users.noreply.github.com> Date: Tue, 7 Feb 2023 07:45:00 +0000 Subject: [PATCH 4/9] Added a setter to GitHub.DistributedTask.WebApi.Issue's key-value-pair indexer. --- src/Runner.Common/JobServerQueue.cs | 5 ++--- src/Runner.Listener/JobDispatcher.cs | 10 +++++----- src/Runner.Worker/ExecutionContext.cs | 13 ++++++++++--- src/Sdk/DTWebApi/WebApi/Issue.cs | 26 ++++++++++---------------- 4 files changed, 27 insertions(+), 27 deletions(-) diff --git a/src/Runner.Common/JobServerQueue.cs b/src/Runner.Common/JobServerQueue.cs index d84614c15c3..dd8907ebeef 100644 --- a/src/Runner.Common/JobServerQueue.cs +++ b/src/Runner.Common/JobServerQueue.cs @@ -299,7 +299,7 @@ private async Task ProcessWebConsoleLinesQueueAsync(bool runOnce = false) { try { - // Give at most 60s for each request. + // Give at most 60s for each request. using (var timeoutTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(60))) { await _jobServer.AppendTimelineRecordFeedAsync(_scopeIdentifier, _hubName, _planId, _jobTimelineId, _jobTimelineRecordId, stepRecordId, batch.Select(logLine => logLine.Line).ToList(), batch[0].LineNumber, timeoutTokenSource.Token); @@ -600,8 +600,7 @@ private List MergeTimelineRecords(List timelineR { foreach (var issue in record.Issues) { - String source; - issue.Data.TryGetValue("sourcepath", out source); + string source = issue["sourcepath"]; Trace.Verbose($" Issue: c={issue.Category}, t={issue.Type}, s={source ?? string.Empty}, m={issue.Message}"); } } diff --git a/src/Runner.Listener/JobDispatcher.cs b/src/Runner.Listener/JobDispatcher.cs index aaa83b80c78..12a8b26d7f7 100644 --- a/src/Runner.Listener/JobDispatcher.cs +++ b/src/Runner.Listener/JobDispatcher.cs @@ -33,7 +33,7 @@ public interface IJobDispatcher : IRunnerService // This implementation of IJobDispatcher is not thread safe. // It is based on the fact that the current design of the runner is a dequeue // and processes one message from the message queue at a time. - // In addition, it only executes one job every time, + // In addition, it only executes one job every time, // and the server will not send another job while this one is still running. public sealed class JobDispatcher : RunnerService, IJobDispatcher { @@ -426,7 +426,7 @@ private async Task RunAsync(Pipelines.AgentJobRequestMessage message, string orc { workerOutput.Add(stdout.Data); } - + if (printToStdout) { term.WriteLine(stdout.Data, skipTracing: true); @@ -512,7 +512,7 @@ await processChannel.SendAsync( var accessToken = systemConnection?.Authorization?.Parameters["AccessToken"]; notification.JobStarted(message.JobId, accessToken, systemConnection.Url); - HostContext.WritePerfCounter($"SentJobToWorker_{requestId.ToString()}"); + HostContext.WritePerfCounter($"SentJobToWorker_{requestId}"); try { @@ -620,7 +620,7 @@ await processChannel.SendAsync( } } - // wait worker to exit + // wait worker to exit // if worker doesn't exit within timeout, then kill worker. completedTask = await Task.WhenAny(workerProcessTask, Task.Delay(-1, workerCancelTimeoutKillToken)); @@ -1014,7 +1014,7 @@ private async Task LogWorkerProcessUnhandledException(IJobServer jobServer, Pipe } var unhandledExceptionIssue = new Issue() { Type = IssueType.Error, Message = errorMessage }; - unhandledExceptionIssue.Data[Constants.Runner.InternalTelemetryIssueDataKey] = Constants.Runner.WorkerCrash; + unhandledExceptionIssue[Constants.Runner.InternalTelemetryIssueDataKey] = Constants.Runner.WorkerCrash; jobRecord.ErrorCount++; jobRecord.Issues.Add(unhandledExceptionIssue); await jobServer.UpdateTimelineRecordsAsync(message.Plan.ScopeIdentifier, message.Plan.PlanType, message.Plan.PlanId, message.Timeline.Id, new TimelineRecord[] { jobRecord }, CancellationToken.None); diff --git a/src/Runner.Worker/ExecutionContext.cs b/src/Runner.Worker/ExecutionContext.cs index 1f7c6e6206a..9fb0786af08 100644 --- a/src/Runner.Worker/ExecutionContext.cs +++ b/src/Runner.Worker/ExecutionContext.cs @@ -591,13 +591,20 @@ public IReadOnlyIssue CreateIssue(IssueType issueType, string rawMessage, IssueM Message = refinedMessage, }; - result.Data.AddRangeIfRangeNotNull(metadata?.Data); + + if (metadata != null) + { + foreach (var kvp in metadata.Data) + { + result[kvp.Key] = kvp.Value; + } + } // It's important to keep track of the step number (key:stepNumber) and the line number (key:logFileLineNumber) of every issue that gets logged. // Actions UI from the run summary page use both values to easily link to an exact locations in logs where annotations originate from. if (_record.Order != null) { - result.Data["stepNumber"] = _record.Order.ToString(); + result["stepNumber"] = _record.Order.ToString(); } string wellKnownTag = null; @@ -627,7 +634,7 @@ public IReadOnlyIssue CreateIssue(IssueType issueType, string rawMessage, IssueM if (!string.IsNullOrEmpty(logText)) { long logLineNumber = Write(wellKnownTag, logText); - result.Data["logFileLineNumber"] = logLineNumber.ToString(); + result["logFileLineNumber"] = logLineNumber.ToString(); } } if (previousCountForIssueType.GetValueOrDefault(0) < _maxIssueCount) diff --git a/src/Sdk/DTWebApi/WebApi/Issue.cs b/src/Sdk/DTWebApi/WebApi/Issue.cs index c9dbbd789e2..337103d3d00 100644 --- a/src/Sdk/DTWebApi/WebApi/Issue.cs +++ b/src/Sdk/DTWebApi/WebApi/Issue.cs @@ -5,7 +5,6 @@ namespace GitHub.DistributedTask.WebApi { - public interface IReadOnlyIssue { IssueType Type { get; } @@ -26,14 +25,14 @@ public Issue() private Issue(Issue original) { - m_data = new Dictionary(StringComparer.OrdinalIgnoreCase); + m_data = new Dictionary(StringComparer.OrdinalIgnoreCase); if (original != null) { this.Type = original.Type; this.Category = original.Category; this.Message = original.Message; this.IsInfrastructureIssue = original.IsInfrastructureIssue; - this.Data.AddRange(original.Data); + m_data.AddRange(original.m_data); } } @@ -45,14 +44,14 @@ public IssueType Type } [DataMember(Order = 2)] - public String Category + public string Category { get; set; } [DataMember(Order = 3)] - public String Message + public string Message { get; set; @@ -65,14 +64,6 @@ public bool? IsInfrastructureIssue set; } - public IDictionary Data - { - get - { - return m_data; - } - } - public string this[string key] { get @@ -80,9 +71,12 @@ public string this[string key] m_data.TryGetValue(key, out string result); return result; } + set + { + m_data[key] = value; + } } - public Issue Clone() { return new Issue(this); @@ -107,8 +101,8 @@ private void OnSerialized(StreamingContext context) } [DataMember(Name = "Data", EmitDefaultValue = false, Order = 4)] - private IDictionary m_serializedData; + private IDictionary m_serializedData; - private IDictionary m_data; + private IDictionary m_data; } } From 14096a7ee46e9612b3729d33557e9b4d5f4eb89b Mon Sep 17 00:00:00 2001 From: John Wesley Walker III <81404201+jww3@users.noreply.github.com> Date: Wed, 8 Feb 2023 13:33:26 +0000 Subject: [PATCH 5/9] Fixed/simplified unit tests. Close-over KeyValuePairs passed via IssueMetadata to ensure no deferred evaluation. (An `IEnumerable>` could very well be a mutable dictionary, so we want to capture its content in the moment -- not some future version of it.) --- src/Runner.Worker/ExecutionContext.cs | 5 ++-- src/Runner.Worker/IssueMatcher.cs | 5 +++- src/Test/L0/TestUtil.cs | 23 +++++++++++++++++++ src/Test/L0/Worker/ActionCommandManagerL0.cs | 15 +----------- src/Test/L0/Worker/ActionManagerL0.cs | 13 +---------- src/Test/L0/Worker/ActionManifestManagerL0.cs | 14 +---------- src/Test/L0/Worker/ActionRunnerL0.cs | 14 +---------- .../L0/Worker/CreateStepSummaryCommandL0.cs | 14 +---------- src/Test/L0/Worker/ExecutionContextL0.cs | 6 ++--- src/Test/L0/Worker/OutputManagerL0.cs | 14 +---------- 10 files changed, 38 insertions(+), 85 deletions(-) diff --git a/src/Runner.Worker/ExecutionContext.cs b/src/Runner.Worker/ExecutionContext.cs index 9fb0786af08..4bfcfa5f75b 100644 --- a/src/Runner.Worker/ExecutionContext.cs +++ b/src/Runner.Worker/ExecutionContext.cs @@ -586,14 +586,13 @@ public IReadOnlyIssue CreateIssue(IssueType issueType, string rawMessage, IssueM var result = new Issue() { Type = issueType, - Category = metadata?.Category, - IsInfrastructureIssue = metadata?.IsInfrastructureIssue ?? false, Message = refinedMessage, }; - if (metadata != null) { + result.Category = metadata.Category; + result.IsInfrastructureIssue = metadata.IsInfrastructureIssue; foreach (var kvp in metadata.Data) { result[kvp.Key] = kvp.Value; diff --git a/src/Runner.Worker/IssueMatcher.cs b/src/Runner.Worker/IssueMatcher.cs index a1e85fed5f8..f55a49de6ff 100644 --- a/src/Runner.Worker/IssueMatcher.cs +++ b/src/Runner.Worker/IssueMatcher.cs @@ -21,7 +21,10 @@ public IssueMetadata(string category, bool infrastructureIssue, string logMessag this.Category = category; this.IsInfrastructureIssue = infrastructureIssue; this.LogMessageOverride = logMessageOverride; - this.Data = data; + + // Close-over the incoming IEnumerable to force immediate evaluation. + var empty = Enumerable.Empty>(); + this.Data = new Dictionary(data ?? empty); } public readonly string Category; diff --git a/src/Test/L0/TestUtil.cs b/src/Test/L0/TestUtil.cs index c6c60be56a7..1f245865551 100644 --- a/src/Test/L0/TestUtil.cs +++ b/src/Test/L0/TestUtil.cs @@ -2,6 +2,8 @@ using Xunit; using GitHub.Runner.Sdk; using System.Runtime.CompilerServices; +using GitHub.DistributedTask.WebApi; +using GitHub.Runner.Worker; namespace GitHub.Runner.Common.Tests { @@ -41,5 +43,26 @@ public static string GetTestDataPath() Assert.True(Directory.Exists(testDataDir)); return testDataDir; } + + public static IReadOnlyIssue CreateTestIssue(IssueType type, string message, IssueMetadata metadata, bool writeToLog) + { + var result = new Issue() + { + Type = type, + Message = message, + }; + + if (metadata != null) + { + result.Category = metadata.Category; + result.IsInfrastructureIssue = metadata.IsInfrastructureIssue; + foreach (var kvp in metadata.Data) + { + result[kvp.Key] = kvp.Value; + } + } + + return result; + } } } diff --git a/src/Test/L0/Worker/ActionCommandManagerL0.cs b/src/Test/L0/Worker/ActionCommandManagerL0.cs index a94413cb8a3..63cfea9e3a2 100644 --- a/src/Test/L0/Worker/ActionCommandManagerL0.cs +++ b/src/Test/L0/Worker/ActionCommandManagerL0.cs @@ -6,7 +6,6 @@ using GitHub.DistributedTask.WebApi; using GitHub.Runner.Worker; using GitHub.Runner.Worker.Container; -using GitHub.Services.Common; using Moq; using Xunit; using Pipelines = GitHub.DistributedTask.Pipelines; @@ -94,19 +93,7 @@ public void StopProcessCommand() }); _ec.Setup(x => x.CreateIssue(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) - .Returns((IssueType type, string message, IssueMetadata metadata, bool writeToLog) => - { - var result = new Issue() - { - Type = type, - Message = message, - Category = metadata?.Category, - IsInfrastructureIssue = metadata?.IsInfrastructureIssue ?? false - }; - result.Data.AddRangeIfRangeNotNull(metadata?.Data); - return result; - }); - + .Returns(TestUtil.CreateTestIssue); _ec.Setup(x => x.AddIssue(It.IsAny())) .Callback((IReadOnlyIssue issue) => { diff --git a/src/Test/L0/Worker/ActionManagerL0.cs b/src/Test/L0/Worker/ActionManagerL0.cs index 1d14bf5ed5a..8d25c9039e5 100644 --- a/src/Test/L0/Worker/ActionManagerL0.cs +++ b/src/Test/L0/Worker/ActionManagerL0.cs @@ -2148,18 +2148,7 @@ private void Setup([CallerMemberName] string name = "", bool enableComposite = t _ec.Object.Global.Plan = new TaskOrchestrationPlanReference(); _ec.Setup(x => x.Write(It.IsAny(), It.IsAny())).Callback((string tag, string message) => { _hc.GetTrace().Info($"[{tag}]{message}"); }); _ec.Setup(x => x.CreateIssue(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) - .Returns((IssueType type, string message, IssueMetadata metadata, bool writeToLog) => - { - var result = new Issue() - { - Type = type, - Message = message, - Category = metadata?.Category, - IsInfrastructureIssue = metadata?.IsInfrastructureIssue ?? false - }; - result.Data.AddRangeIfRangeNotNull(metadata?.Data); - return result; - }); + .Returns(TestUtil.CreateTestIssue); _ec.Setup(x => x.AddIssue(It.IsAny())).Callback((IReadOnlyIssue issue) => { _hc.GetTrace().Info($"[{issue.Type}]{issue.Message}"); }); _ec.Setup(x => x.GetGitHubContext("workspace")).Returns(Path.Combine(_workFolder, "actions", "actions")); diff --git a/src/Test/L0/Worker/ActionManifestManagerL0.cs b/src/Test/L0/Worker/ActionManifestManagerL0.cs index aaec5f772dd..cd357d16961 100644 --- a/src/Test/L0/Worker/ActionManifestManagerL0.cs +++ b/src/Test/L0/Worker/ActionManifestManagerL0.cs @@ -862,19 +862,7 @@ private void Setup([CallerMemberName] string name = "") _ec.Setup(x => x.Write(It.IsAny(), It.IsAny())).Callback((string tag, string message) => { _hc.GetTrace().Info($"{tag}{message}"); }); _ec.Setup(x => x.CreateIssue(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) - .Returns((IssueType type, string message, IssueMetadata metadata, bool writeToLog) => - { - var result = new Issue() - { - Type = type, - Message = message, - Category = metadata?.Category, - IsInfrastructureIssue = metadata?.IsInfrastructureIssue ?? false - }; - result.Data.AddRangeIfRangeNotNull(metadata?.Data); - return result; - }); - + .Returns(TestUtil.CreateTestIssue); _ec.Setup(x => x.AddIssue(It.IsAny())).Callback((IReadOnlyIssue issue) => { _hc.GetTrace().Info($"[{issue.Type}]{issue.Message}"); }); } diff --git a/src/Test/L0/Worker/ActionRunnerL0.cs b/src/Test/L0/Worker/ActionRunnerL0.cs index f56f8553699..236cba3d04d 100644 --- a/src/Test/L0/Worker/ActionRunnerL0.cs +++ b/src/Test/L0/Worker/ActionRunnerL0.cs @@ -445,19 +445,7 @@ private void Setup([CallerMemberName] string name = "") _ec.Object.Global.Variables = new Variables(_hc, new Dictionary()); _ec.Setup(x => x.Write(It.IsAny(), It.IsAny())).Callback((string tag, string message) => { _hc.GetTrace().Info($"[{tag}]{message}"); }); _ec.Setup(x => x.CreateIssue(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) - .Returns((IssueType type, string message, IssueMetadata metadata, bool writeToLog) => - { - var result = new Issue() - { - Type = type, - Message = message, - Category = metadata?.Category, - IsInfrastructureIssue = metadata?.IsInfrastructureIssue ?? false - }; - result.Data.AddRangeIfRangeNotNull(metadata?.Data); - return result; - }); - + .Returns(TestUtil.CreateTestIssue); _ec.Setup(x => x.AddIssue(It.IsAny())).Callback((IReadOnlyIssue issue) => { _hc.GetTrace().Info($"[{issue.Type}]{issue.Message}"); }); _hc.SetSingleton(_actionManager.Object); diff --git a/src/Test/L0/Worker/CreateStepSummaryCommandL0.cs b/src/Test/L0/Worker/CreateStepSummaryCommandL0.cs index 69b2cbdda62..38acb1e2ab2 100644 --- a/src/Test/L0/Worker/CreateStepSummaryCommandL0.cs +++ b/src/Test/L0/Worker/CreateStepSummaryCommandL0.cs @@ -255,19 +255,7 @@ private TestHostContext Setup([CallerMemberName] string name = "") _trace.Info($"Issue '{issue.Type}': {issue.Message}"); }); _executionContext.Setup(x => x.CreateIssue(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) - .Returns((IssueType type, string message, IssueMetadata metadata, bool writeToLog) => - { - var result = new Issue() - { - Type = type, - Message = message, - Category = metadata?.Category, - IsInfrastructureIssue = metadata?.IsInfrastructureIssue ?? false - }; - result.Data.AddRangeIfRangeNotNull(metadata?.Data); - return result; - }); - + .Returns(TestUtil.CreateTestIssue); _executionContext.Setup(x => x.Write(It.IsAny(), It.IsAny())) .Callback((string tag, string message) => { diff --git a/src/Test/L0/Worker/ExecutionContextL0.cs b/src/Test/L0/Worker/ExecutionContextL0.cs index 91adbfb182a..216737ab595 100644 --- a/src/Test/L0/Worker/ExecutionContextL0.cs +++ b/src/Test/L0/Worker/ExecutionContextL0.cs @@ -246,9 +246,9 @@ public void AddIssue_AddStepAndLineNumberInformation() embeddedStep.AddIssue(embeddedStep.CreateIssue(IssueType.Warning, "warning annotation that should have step and line number information", null, true)); embeddedStep.AddIssue(embeddedStep.CreateIssue(IssueType.Notice, "notice annotation that should have step and line number information", null, true)); - jobServerQueue.Verify(x => x.QueueTimelineRecordUpdate(It.IsAny(), It.Is(t => t.Issues.Where(i => i.Data.ContainsKey("stepNumber") && i.Data.ContainsKey("logFileLineNumber") && i.Type == IssueType.Error).Count() == 1)), Times.AtLeastOnce); - jobServerQueue.Verify(x => x.QueueTimelineRecordUpdate(It.IsAny(), It.Is(t => t.Issues.Where(i => i.Data.ContainsKey("stepNumber") && i.Data.ContainsKey("logFileLineNumber") && i.Type == IssueType.Warning).Count() == 1)), Times.AtLeastOnce); - jobServerQueue.Verify(x => x.QueueTimelineRecordUpdate(It.IsAny(), It.Is(t => t.Issues.Where(i => i.Data.ContainsKey("stepNumber") && i.Data.ContainsKey("logFileLineNumber") && i.Type == IssueType.Notice).Count() == 1)), Times.AtLeastOnce); + jobServerQueue.Verify(x => x.QueueTimelineRecordUpdate(It.IsAny(), It.Is(t => t.Issues.Where(i => i["stepNumber"] != null && i["logFileLineNumber"] != null && i.Type == IssueType.Error).Count() == 1)), Times.AtLeastOnce); + jobServerQueue.Verify(x => x.QueueTimelineRecordUpdate(It.IsAny(), It.Is(t => t.Issues.Where(i => i["stepNumber"] != null && i["logFileLineNumber"] != null && i.Type == IssueType.Warning).Count() == 1)), Times.AtLeastOnce); + jobServerQueue.Verify(x => x.QueueTimelineRecordUpdate(It.IsAny(), It.Is(t => t.Issues.Where(i => i["stepNumber"] != null && i["logFileLineNumber"] != null && i.Type == IssueType.Notice).Count() == 1)), Times.AtLeastOnce); } } diff --git a/src/Test/L0/Worker/OutputManagerL0.cs b/src/Test/L0/Worker/OutputManagerL0.cs index 09d98d96557..9b74295e247 100644 --- a/src/Test/L0/Worker/OutputManagerL0.cs +++ b/src/Test/L0/Worker/OutputManagerL0.cs @@ -984,19 +984,7 @@ private TestHostContext Setup( _onMatcherChanged = handler; }); _executionContext.Setup(x => x.CreateIssue(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) - .Returns((DTWebApi.IssueType type, string message, IssueMetadata metadata, bool writeToLog) => - { - var result = new DTWebApi.Issue() - { - Type = type, - Message = message, - Category = metadata?.Category, - IsInfrastructureIssue = metadata?.IsInfrastructureIssue ?? false - }; - result.Data.AddRangeIfRangeNotNull(metadata?.Data); - return result; - }); - + .Returns(TestUtil.CreateTestIssue); _executionContext.Setup(x => x.AddIssue(It.IsAny())) .Callback((DTWebApi.IReadOnlyIssue issue) => { From b70f97f183f20126c0e2243317354334c0992ed8 Mon Sep 17 00:00:00 2001 From: John Wesley Walker III <81404201+jww3@users.noreply.github.com> Date: Wed, 8 Feb 2023 14:15:14 +0000 Subject: [PATCH 6/9] Restored a using directive that proved to be necessary. In some cases, references to `System.IO.Compression` members are #ifdef'd and only needed when the OS_WINDOWS compiler directive is present. --- src/Runner.Worker/ActionManager.cs | 2 +- src/Runner.Worker/DiagnosticLogManager.cs | 3 +-- src/Test/L0/Worker/ActionManagerL0.cs | 6 +++++- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/Runner.Worker/ActionManager.cs b/src/Runner.Worker/ActionManager.cs index 49e14e68ae3..6014dc32026 100644 --- a/src/Runner.Worker/ActionManager.cs +++ b/src/Runner.Worker/ActionManager.cs @@ -1,7 +1,7 @@ using System; using System.Collections.Generic; using System.IO; -using System.IO.Compression; +using System.IO.Compression; // required for OS_WINDOWS using System.Linq; using System.Net; using System.Net.Http; diff --git a/src/Runner.Worker/DiagnosticLogManager.cs b/src/Runner.Worker/DiagnosticLogManager.cs index 698f2112c16..278a1f8eaae 100644 --- a/src/Runner.Worker/DiagnosticLogManager.cs +++ b/src/Runner.Worker/DiagnosticLogManager.cs @@ -5,7 +5,6 @@ using GitHub.DistributedTask.WebApi; using System.Linq; using System.Globalization; -using System.Threading.Tasks; using Pipelines = GitHub.DistributedTask.Pipelines; using GitHub.Runner.Common; using GitHub.Runner.Sdk; @@ -32,7 +31,7 @@ public sealed class DiagnosticLogManager : RunnerService, IDiagnosticLogManager { private static string DateTimeFormat = "yyyyMMdd-HHmmss"; public void UploadDiagnosticLogs(IExecutionContext executionContext, - IExecutionContext parentContext, + IExecutionContext parentContext, Pipelines.AgentJobRequestMessage message, DateTime jobStartTimeUtc) { diff --git a/src/Test/L0/Worker/ActionManagerL0.cs b/src/Test/L0/Worker/ActionManagerL0.cs index 8d25c9039e5..093305f33bf 100644 --- a/src/Test/L0/Worker/ActionManagerL0.cs +++ b/src/Test/L0/Worker/ActionManagerL0.cs @@ -13,7 +13,11 @@ using GitHub.Runner.Sdk; using GitHub.Runner.Worker; using GitHub.Runner.Worker.Container; -using GitHub.Services.Common; + +#if OS_WINDOWS +using System.IO.Compression; +#endif + using Moq; using Moq.Protected; using Xunit; From cb89be7aac3c1031d85f0c1db0ebb65ba9b197b2 Mon Sep 17 00:00:00 2001 From: John Wesley Walker III <81404201+jww3@users.noreply.github.com> Date: Mon, 27 Feb 2023 16:38:45 +0000 Subject: [PATCH 7/9] Account for DataContractSerializer vagaries. --- src/Sdk/DTWebApi/WebApi/Issue.cs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/Sdk/DTWebApi/WebApi/Issue.cs b/src/Sdk/DTWebApi/WebApi/Issue.cs index 337103d3d00..d5ce09492c7 100644 --- a/src/Sdk/DTWebApi/WebApi/Issue.cs +++ b/src/Sdk/DTWebApi/WebApi/Issue.cs @@ -25,7 +25,8 @@ public Issue() private Issue(Issue original) { - m_data = new Dictionary(StringComparer.OrdinalIgnoreCase); + // DataContractSerializer bypasses all constructor logic and inline initialization! + this.EnsureInitialized(); if (original != null) { this.Type = original.Type; @@ -86,6 +87,7 @@ public Issue Clone() private void OnDeserialized(StreamingContext context) { SerializationHelper.Copy(ref m_serializedData, ref m_data, StringComparer.OrdinalIgnoreCase, true); + this.EnsureInitialized(); } [OnSerializing] @@ -104,5 +106,10 @@ private void OnSerialized(StreamingContext context) private IDictionary m_serializedData; private IDictionary m_data; + + private void EnsureInitialized() + { + m_data = m_data ?? new Dictionary(StringComparer.OrdinalIgnoreCase); + } } } From f3961c48957726dd11078482b9d2e33f52e4c401 Mon Sep 17 00:00:00 2001 From: John Wesley Walker III <81404201+jww3@users.noreply.github.com> Date: Mon, 27 Feb 2023 17:45:57 +0000 Subject: [PATCH 8/9] Refined deserialization fixup. --- src/Runner.Worker/IssueMatcher.cs | 2 +- src/Sdk/DTWebApi/WebApi/Issue.cs | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/Runner.Worker/IssueMatcher.cs b/src/Runner.Worker/IssueMatcher.cs index f55a49de6ff..752a5d86a04 100644 --- a/src/Runner.Worker/IssueMatcher.cs +++ b/src/Runner.Worker/IssueMatcher.cs @@ -24,7 +24,7 @@ public IssueMetadata(string category, bool infrastructureIssue, string logMessag // Close-over the incoming IEnumerable to force immediate evaluation. var empty = Enumerable.Empty>(); - this.Data = new Dictionary(data ?? empty); + this.Data = new Dictionary(data ?? empty, StringComparer.OrdinalIgnoreCase); } public readonly string Category; diff --git a/src/Sdk/DTWebApi/WebApi/Issue.cs b/src/Sdk/DTWebApi/WebApi/Issue.cs index d5ce09492c7..3566a3d5492 100644 --- a/src/Sdk/DTWebApi/WebApi/Issue.cs +++ b/src/Sdk/DTWebApi/WebApi/Issue.cs @@ -25,7 +25,6 @@ public Issue() private Issue(Issue original) { - // DataContractSerializer bypasses all constructor logic and inline initialization! this.EnsureInitialized(); if (original != null) { @@ -107,6 +106,11 @@ private void OnSerialized(StreamingContext context) private IDictionary m_data; + /// + /// DataContractSerializer bypasses all constructor logic and inline initialization! + /// This method takes the place of a workhorse constructor for baseline initialization. + /// The expectation is for this logic to be accessible to constructors and also to the OnDeserialized helper. + /// private void EnsureInitialized() { m_data = m_data ?? new Dictionary(StringComparer.OrdinalIgnoreCase); From e3e42889af1d1a606e5a905ceeeca71982cb5d5b Mon Sep 17 00:00:00 2001 From: John Wesley Walker III <81404201+jww3@users.noreply.github.com> Date: Mon, 27 Feb 2023 20:36:22 +0000 Subject: [PATCH 9/9] Keep Issue.cs and TimelineRecord.cs aligned in terms of instantiation flow. --- src/Runner.Worker/ActionCommandManager.cs | 3 +- src/Sdk/DTWebApi/WebApi/Issue.cs | 12 +- src/Sdk/DTWebApi/WebApi/TimelineRecord.cs | 146 ++++++++++++---------- 3 files changed, 90 insertions(+), 71 deletions(-) diff --git a/src/Runner.Worker/ActionCommandManager.cs b/src/Runner.Worker/ActionCommandManager.cs index 1691d0eb09d..e77bf81369f 100644 --- a/src/Runner.Worker/ActionCommandManager.cs +++ b/src/Runner.Worker/ActionCommandManager.cs @@ -646,7 +646,8 @@ public void ProcessCommand(IExecutionContext context, string inputLine, ActionCo string keyToExclude = Constants.Runner.InternalTelemetryIssueDataKey; var filteredDictionaryEntries = command.Properties - .Where(kvp => !string.Equals(kvp.Key, keyToExclude, StringComparison.OrdinalIgnoreCase)); + .Where(kvp => !string.Equals(kvp.Key, keyToExclude, StringComparison.OrdinalIgnoreCase)) + .ToList(); var metadata = new IssueMetadata(issueCategory, false, null, filteredDictionaryEntries); var issue = context.CreateIssue(this.Type, command.Data, metadata, true); diff --git a/src/Sdk/DTWebApi/WebApi/Issue.cs b/src/Sdk/DTWebApi/WebApi/Issue.cs index 3566a3d5492..2235dd86135 100644 --- a/src/Sdk/DTWebApi/WebApi/Issue.cs +++ b/src/Sdk/DTWebApi/WebApi/Issue.cs @@ -101,11 +101,6 @@ private void OnSerialized(StreamingContext context) m_serializedData = null; } - [DataMember(Name = "Data", EmitDefaultValue = false, Order = 4)] - private IDictionary m_serializedData; - - private IDictionary m_data; - /// /// DataContractSerializer bypasses all constructor logic and inline initialization! /// This method takes the place of a workhorse constructor for baseline initialization. @@ -113,7 +108,14 @@ private void OnSerialized(StreamingContext context) /// private void EnsureInitialized() { + //Note that ?? is a short-circuiting operator. m_data = m_data ?? new Dictionary(StringComparer.OrdinalIgnoreCase); } + + [DataMember(Name = "Data", EmitDefaultValue = false, Order = 4)] + private IDictionary m_serializedData; + + private IDictionary m_data; + } } diff --git a/src/Sdk/DTWebApi/WebApi/TimelineRecord.cs b/src/Sdk/DTWebApi/WebApi/TimelineRecord.cs index 51317d1713b..e42607e1019 100644 --- a/src/Sdk/DTWebApi/WebApi/TimelineRecord.cs +++ b/src/Sdk/DTWebApi/WebApi/TimelineRecord.cs @@ -10,69 +10,76 @@ namespace GitHub.DistributedTask.WebApi public sealed class TimelineRecord { public TimelineRecord() + : this(null) { - this.Attempt = 1; } private TimelineRecord(TimelineRecord recordToBeCloned) { - this.Attempt = recordToBeCloned.Attempt; - this.ChangeId = recordToBeCloned.ChangeId; - this.CurrentOperation = recordToBeCloned.CurrentOperation; - this.FinishTime = recordToBeCloned.FinishTime; - this.Id = recordToBeCloned.Id; - this.Identifier = recordToBeCloned.Identifier; - this.LastModified = recordToBeCloned.LastModified; - this.Location = recordToBeCloned.Location; - this.Name = recordToBeCloned.Name; - this.Order = recordToBeCloned.Order; - this.ParentId = recordToBeCloned.ParentId; - this.PercentComplete = recordToBeCloned.PercentComplete; - this.RecordType = recordToBeCloned.RecordType; - this.Result = recordToBeCloned.Result; - this.ResultCode = recordToBeCloned.ResultCode; - this.StartTime = recordToBeCloned.StartTime; - this.State = recordToBeCloned.State; - this.TimelineId = recordToBeCloned.TimelineId; - this.WorkerName = recordToBeCloned.WorkerName; - this.RefName = recordToBeCloned.RefName; - this.ErrorCount = recordToBeCloned.ErrorCount; - this.WarningCount = recordToBeCloned.WarningCount; - this.NoticeCount = recordToBeCloned.NoticeCount; - this.AgentPlatform = recordToBeCloned.AgentPlatform; - - if (recordToBeCloned.Log != null) + this.EnsureInitialized(); + if (recordToBeCloned != null) { - this.Log = new TaskLogReference + this.Attempt = recordToBeCloned.Attempt; + this.ChangeId = recordToBeCloned.ChangeId; + this.CurrentOperation = recordToBeCloned.CurrentOperation; + this.FinishTime = recordToBeCloned.FinishTime; + this.Id = recordToBeCloned.Id; + this.Identifier = recordToBeCloned.Identifier; + this.LastModified = recordToBeCloned.LastModified; + this.Location = recordToBeCloned.Location; + this.Name = recordToBeCloned.Name; + this.Order = recordToBeCloned.Order; + this.ParentId = recordToBeCloned.ParentId; + this.PercentComplete = recordToBeCloned.PercentComplete; + this.RecordType = recordToBeCloned.RecordType; + this.Result = recordToBeCloned.Result; + this.ResultCode = recordToBeCloned.ResultCode; + this.StartTime = recordToBeCloned.StartTime; + this.State = recordToBeCloned.State; + this.TimelineId = recordToBeCloned.TimelineId; + this.WorkerName = recordToBeCloned.WorkerName; + this.RefName = recordToBeCloned.RefName; + this.ErrorCount = recordToBeCloned.ErrorCount; + this.WarningCount = recordToBeCloned.WarningCount; + this.NoticeCount = recordToBeCloned.NoticeCount; + this.AgentPlatform = recordToBeCloned.AgentPlatform; + + if (recordToBeCloned.Log != null) { - Id = recordToBeCloned.Log.Id, - Location = recordToBeCloned.Log.Location, - }; - } + this.Log = new TaskLogReference + { + Id = recordToBeCloned.Log.Id, + Location = recordToBeCloned.Log.Location, + }; + } - if (recordToBeCloned.Details != null) - { - this.Details = new TimelineReference + if (recordToBeCloned.Details != null) { - ChangeId = recordToBeCloned.Details.ChangeId, - Id = recordToBeCloned.Details.Id, - Location = recordToBeCloned.Details.Location, - }; - } + this.Details = new TimelineReference + { + ChangeId = recordToBeCloned.Details.ChangeId, + Id = recordToBeCloned.Details.Id, + Location = recordToBeCloned.Details.Location, + }; + } - if (recordToBeCloned.m_issues?.Count> 0) - { - this.Issues.AddRange(recordToBeCloned.Issues.Select(i => i.Clone())); - } + if (recordToBeCloned.m_issues?.Count > 0) + { + m_issues.AddRange(recordToBeCloned.m_issues.Select(i => i.Clone())); + } - if (recordToBeCloned.m_previousAttempts?.Count > 0) - { - this.PreviousAttempts.AddRange(recordToBeCloned.PreviousAttempts); - } + if (recordToBeCloned.m_previousAttempts?.Count > 0) + { + m_previousAttempts.AddRange(recordToBeCloned.m_previousAttempts); + } - if (recordToBeCloned.m_variables?.Count > 0) - { - this.m_variables = recordToBeCloned.Variables.ToDictionary(k => k.Key, v => v.Value.Clone()); + if (recordToBeCloned.m_variables?.Count > 0) + { + // Don't pave over the case-insensitive Dictionary we initialized above. + foreach (var kvp in recordToBeCloned.m_variables) { + m_variables[kvp.Key] = kvp.Value.Clone(); + } + } } } @@ -234,10 +241,6 @@ public List Issues { get { - if (m_issues == null) - { - m_issues = new List(); - } return m_issues; } } @@ -274,22 +277,14 @@ public IList PreviousAttempts { get { - if (m_previousAttempts == null) - { - m_previousAttempts = new List(); - } return m_previousAttempts; } } - public IDictionary Variables + public IDictionary Variables { get { - if (m_variables == null) - { - m_variables = new Dictionary(StringComparer.OrdinalIgnoreCase); - } return m_variables; } } @@ -299,11 +294,32 @@ public TimelineRecord Clone() return new TimelineRecord(this); } + [OnDeserialized] + private void OnDeserialized(StreamingContext context) + { + this.EnsureInitialized(); + } + + /// + /// DataContractSerializer bypasses all constructor logic and inline initialization! + /// This method takes the place of a workhorse constructor for baseline initialization. + /// The expectation is for this logic to be accessible to constructors and also to the OnDeserialized helper. + /// + private void EnsureInitialized() + { + //Note that ?? is a short-circuiting operator. + m_issues = m_issues ?? new List(); + m_variables = m_variables ?? new Dictionary(StringComparer.OrdinalIgnoreCase); + m_previousAttempts = m_previousAttempts ?? new List(); + this.Attempt = Math.Max(this.Attempt, 1); + } + + [DataMember(Name = "Issues", EmitDefaultValue = false, Order = 60)] private List m_issues; [DataMember(Name = "Variables", EmitDefaultValue = false, Order = 80)] - private Dictionary m_variables; + private Dictionary m_variables; [DataMember(Name = "PreviousAttempts", EmitDefaultValue = false, Order = 120)] private List m_previousAttempts;