From 25f6cb1d7852930c314b9ad900f99df370881ab5 Mon Sep 17 00:00:00 2001 From: Sven Pfleiderer Date: Thu, 20 Jan 2022 17:10:52 -0800 Subject: [PATCH 01/29] First prototype of step summary environment variable --- src/Runner.Worker/StepsRunner.cs | 26 ++++++++++++++++++++++++++ src/runnerversion | 2 +- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/src/Runner.Worker/StepsRunner.cs b/src/Runner.Worker/StepsRunner.cs index 7b7bc971670..5fe4d6586ae 100644 --- a/src/Runner.Worker/StepsRunner.cs +++ b/src/Runner.Worker/StepsRunner.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.IO; using System.Text; using System.Threading; using System.Threading.Tasks; @@ -103,6 +104,10 @@ public async Task RunAsync(IExecutionContext jobContext) // Set GITHUB_ACTION step.ExecutionContext.SetGitHubContext("action", actionStep.Action.Name); + var stepSummaryFilePath = GetStepSummaryPath(step); + step.ExecutionContext.SetGitHubContext("step_summary", stepSummaryFilePath); + envContext["GITHUB_STEP_SUMMARY"] = new StringContextData(stepSummaryFilePath); + try { // Evaluate and merge step env @@ -351,8 +356,29 @@ private async Task RunStepAsync(IStep step, CancellationToken jobCancellationTok private void CompleteStep(IStep step, TaskResult? result = null, string resultCode = null) { var executionContext = step.ExecutionContext; + var stepSummaryFilePath = executionContext.GetGitHubContext("step_summary"); + + Trace.Info($"Reading step summary data from {stepSummaryFilePath}"); + Trace.Info($"File exists: {stepSummaryFilePath} {File.Exists(stepSummaryFilePath)}"); + + var fileStream = new FileStream(stepSummaryFilePath, FileMode.Open, FileAccess.Read, FileShare.Read); + using (var sr = new StreamReader(fileStream)) + { + Trace.Info(sr.ReadToEnd()); + } executionContext.Complete(result, resultCode: resultCode); } + + private string GetStepSummaryPath(IStep step) + { + var tempDirectory = HostContext.GetDirectory(WellKnownDirectory.Temp); + Trace.Info($"Using temp directory '{tempDirectory}'"); + var stepSummaryFilePath = Path.Join(tempDirectory, $"{Guid.NewGuid().ToString()}.md"); + Trace.Info($"Using step summary file '{stepSummaryFilePath}'"); + File.Create(stepSummaryFilePath); + + return stepSummaryFilePath; + } } } diff --git a/src/runnerversion b/src/runnerversion index e392d2b0529..d7e347f6613 100644 --- a/src/runnerversion +++ b/src/runnerversion @@ -1 +1 @@ -2.286.0 +2.286.2 From 412dc5c836c711115a1cf9c8eedc5652915f4700 Mon Sep 17 00:00:00 2001 From: Sven Pfleiderer Date: Fri, 21 Jan 2022 14:32:47 -0800 Subject: [PATCH 02/29] Fix file contention issue --- src/Runner.Worker/StepsRunner.cs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/Runner.Worker/StepsRunner.cs b/src/Runner.Worker/StepsRunner.cs index 5fe4d6586ae..3bce4b0cedc 100644 --- a/src/Runner.Worker/StepsRunner.cs +++ b/src/Runner.Worker/StepsRunner.cs @@ -359,12 +359,11 @@ private void CompleteStep(IStep step, TaskResult? result = null, string resultCo var stepSummaryFilePath = executionContext.GetGitHubContext("step_summary"); Trace.Info($"Reading step summary data from {stepSummaryFilePath}"); - Trace.Info($"File exists: {stepSummaryFilePath} {File.Exists(stepSummaryFilePath)}"); var fileStream = new FileStream(stepSummaryFilePath, FileMode.Open, FileAccess.Read, FileShare.Read); using (var sr = new StreamReader(fileStream)) { - Trace.Info(sr.ReadToEnd()); + Trace.Info($"Step summary data: {sr.ReadToEnd()}"); } executionContext.Complete(result, resultCode: resultCode); @@ -372,11 +371,13 @@ private void CompleteStep(IStep step, TaskResult? result = null, string resultCo private string GetStepSummaryPath(IStep step) { - var tempDirectory = HostContext.GetDirectory(WellKnownDirectory.Temp); - Trace.Info($"Using temp directory '{tempDirectory}'"); - var stepSummaryFilePath = Path.Join(tempDirectory, $"{Guid.NewGuid().ToString()}.md"); + var stepSummaryDirectory = Path.Combine(HostContext.GetDirectory(WellKnownDirectory.Temp), "_step_summary"); + Directory.CreateDirectory(stepSummaryDirectory); + var stepSummaryFilePath = Path.Combine(stepSummaryDirectory, $"{Guid.NewGuid().ToString()}.md"); + Trace.Info($"Using step summary file '{stepSummaryFilePath}'"); - File.Create(stepSummaryFilePath); + var fileStream = File.Create(stepSummaryFilePath); + fileStream.Close(); return stepSummaryFilePath; } From 386fa86f91ddbb9bb3a33adfe9904c62e646ebdf Mon Sep 17 00:00:00 2001 From: Sven Pfleiderer Date: Fri, 21 Jan 2022 16:56:35 -0800 Subject: [PATCH 03/29] Try to simplify cleaning up file references --- src/Runner.Worker/StepsRunner.cs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/Runner.Worker/StepsRunner.cs b/src/Runner.Worker/StepsRunner.cs index 3bce4b0cedc..55577ab7e82 100644 --- a/src/Runner.Worker/StepsRunner.cs +++ b/src/Runner.Worker/StepsRunner.cs @@ -359,8 +359,9 @@ private void CompleteStep(IStep step, TaskResult? result = null, string resultCo var stepSummaryFilePath = executionContext.GetGitHubContext("step_summary"); Trace.Info($"Reading step summary data from {stepSummaryFilePath}"); + Trace.Info($"File exists: {stepSummaryFilePath} {File.Exists(stepSummaryFilePath)}"); - var fileStream = new FileStream(stepSummaryFilePath, FileMode.Open, FileAccess.Read, FileShare.Read); + using (var fileStream = new FileStream(stepSummaryFilePath, FileMode.Open, FileAccess.Read, FileShare.Read)) using (var sr = new StreamReader(fileStream)) { Trace.Info($"Step summary data: {sr.ReadToEnd()}"); @@ -375,9 +376,9 @@ private string GetStepSummaryPath(IStep step) Directory.CreateDirectory(stepSummaryDirectory); var stepSummaryFilePath = Path.Combine(stepSummaryDirectory, $"{Guid.NewGuid().ToString()}.md"); - Trace.Info($"Using step summary file '{stepSummaryFilePath}'"); - var fileStream = File.Create(stepSummaryFilePath); - fileStream.Close(); + using (File.Create(stepSummaryFilePath)) { + Trace.Info($"Using step summary file '{stepSummaryFilePath}'"); + } return stepSummaryFilePath; } From e112193313931b09dcb1212cc1dacb61478694a6 Mon Sep 17 00:00:00 2001 From: Rob Herley Date: Tue, 25 Jan 2022 19:00:12 -0500 Subject: [PATCH 04/29] use step id as md file name, queue file attachment --- src/Runner.Common/Constants.cs | 2 ++ src/Runner.Common/HostContext.cs | 6 ++++ src/Runner.Worker/StepsRunner.cs | 42 ++++++++++++++++------- src/Sdk/DTWebApi/WebApi/TaskAttachment.cs | 6 ++++ 4 files changed, 43 insertions(+), 13 deletions(-) diff --git a/src/Runner.Common/Constants.cs b/src/Runner.Common/Constants.cs index c43aae122b2..300ca334cc1 100644 --- a/src/Runner.Common/Constants.cs +++ b/src/Runner.Common/Constants.cs @@ -9,6 +9,7 @@ public enum WellKnownDirectory Externals, Root, Actions, + StepSummary, Temp, Tools, Update, @@ -196,6 +197,7 @@ public static class Path public static readonly string DiagDirectory = "_diag"; public static readonly string ExternalsDirectory = "externals"; public static readonly string RunnerDiagnosticLogPrefix = "Runner_"; + public static readonly string StepSummaryDirectory = "_step_summary"; public static readonly string TempDirectory = "_temp"; public static readonly string ToolDirectory = "_tool"; public static readonly string UpdateDirectory = "_update"; diff --git a/src/Runner.Common/HostContext.cs b/src/Runner.Common/HostContext.cs index 643c61c8125..df6e36bebe7 100644 --- a/src/Runner.Common/HostContext.cs +++ b/src/Runner.Common/HostContext.cs @@ -243,6 +243,12 @@ public string GetDirectory(WellKnownDirectory directory) path = new DirectoryInfo(GetDirectory(WellKnownDirectory.Bin)).Parent.FullName; break; + case WellKnownDirectory.StepSummary: + path = Path.Combine( + GetDirectory(WellKnownDirectory.Temp), + Constants.Path.StepSummaryDirectory); + break; + case WellKnownDirectory.Temp: path = Path.Combine( GetDirectory(WellKnownDirectory.Work), diff --git a/src/Runner.Worker/StepsRunner.cs b/src/Runner.Worker/StepsRunner.cs index 55577ab7e82..f8ae15b901e 100644 --- a/src/Runner.Worker/StepsRunner.cs +++ b/src/Runner.Worker/StepsRunner.cs @@ -104,7 +104,7 @@ public async Task RunAsync(IExecutionContext jobContext) // Set GITHUB_ACTION step.ExecutionContext.SetGitHubContext("action", actionStep.Action.Name); - var stepSummaryFilePath = GetStepSummaryPath(step); + var stepSummaryFilePath = CreateStepSummaryFile(step); step.ExecutionContext.SetGitHubContext("step_summary", stepSummaryFilePath); envContext["GITHUB_STEP_SUMMARY"] = new StringContextData(stepSummaryFilePath); @@ -356,31 +356,47 @@ private async Task RunStepAsync(IStep step, CancellationToken jobCancellationTok private void CompleteStep(IStep step, TaskResult? result = null, string resultCode = null) { var executionContext = step.ExecutionContext; + var parentContext = executionContext.Root; var stepSummaryFilePath = executionContext.GetGitHubContext("step_summary"); Trace.Info($"Reading step summary data from {stepSummaryFilePath}"); - Trace.Info($"File exists: {stepSummaryFilePath} {File.Exists(stepSummaryFilePath)}"); - using (var fileStream = new FileStream(stepSummaryFilePath, FileMode.Open, FileAccess.Read, FileShare.Read)) - using (var sr = new StreamReader(fileStream)) + var summaryExists = File.Exists(stepSummaryFilePath); + if (summaryExists) { - Trace.Info($"Step summary data: {sr.ReadToEnd()}"); + Trace.Info($"File exists: {stepSummaryFilePath}"); + + var summaryFileIsEmpty = new FileInfo(stepSummaryFilePath).Length == 0; + if (summaryFileIsEmpty) + { + Trace.Info($"Summary file ({summaryFileIsEmpty}) is empty, skipping attachment upload"); + } + else + { + var stepID = executionContext.Id; + Trace.Info($"Queueing file ({stepSummaryFilePath}) for attachment upload ({stepID})"); + parentContext.QueueAttachFile(ChecksAttachmentType.StepSummary, stepID.ToString(), stepSummaryFilePath); + } } executionContext.Complete(result, resultCode: resultCode); } - private string GetStepSummaryPath(IStep step) + private string CreateStepSummaryFile(IStep step) { - var stepSummaryDirectory = Path.Combine(HostContext.GetDirectory(WellKnownDirectory.Temp), "_step_summary"); - Directory.CreateDirectory(stepSummaryDirectory); - var stepSummaryFilePath = Path.Combine(stepSummaryDirectory, $"{Guid.NewGuid().ToString()}.md"); - - using (File.Create(stepSummaryFilePath)) { - Trace.Info($"Using step summary file '{stepSummaryFilePath}'"); + var stepSummaryDirectory = HostContext.GetDirectory(WellKnownDirectory.StepSummary); + if (!Directory.Exists(stepSummaryDirectory)) + { + Trace.Info($"Creating step summary directory: {stepSummaryDirectory}"); + Directory.CreateDirectory(stepSummaryDirectory); } + var stepID = step.ExecutionContext.Id; + var stepSummaryFilePath = Path.Combine(stepSummaryDirectory, $"{stepID}.md"); + + Trace.Info($"Creating step summary file: {stepSummaryFilePath}"); + File.Create(stepSummaryFilePath).Close(); return stepSummaryFilePath; } } -} +} \ No newline at end of file diff --git a/src/Sdk/DTWebApi/WebApi/TaskAttachment.cs b/src/Sdk/DTWebApi/WebApi/TaskAttachment.cs index 0b55a06d296..fac99a417e7 100644 --- a/src/Sdk/DTWebApi/WebApi/TaskAttachment.cs +++ b/src/Sdk/DTWebApi/WebApi/TaskAttachment.cs @@ -102,4 +102,10 @@ public class CoreAttachmentType public static readonly String FileAttachment = "DistributedTask.Core.FileAttachment"; public static readonly String DiagnosticLog = "DistributedTask.Core.DiagnosticLog"; } + + [GenerateAllConstants] + public class ChecksAttachmentType + { + public static readonly String StepSummary = "Checks.Step.Summary"; + } } From e552c0cced4b841c91f4796578451d29e537ff14 Mon Sep 17 00:00:00 2001 From: Rob Herley Date: Tue, 25 Jan 2022 19:03:14 -0500 Subject: [PATCH 05/29] separate logic into attachment summary func --- src/Runner.Worker/StepsRunner.cs | 48 ++++++++++++++++++-------------- 1 file changed, 27 insertions(+), 21 deletions(-) diff --git a/src/Runner.Worker/StepsRunner.cs b/src/Runner.Worker/StepsRunner.cs index f8ae15b901e..fc3d41de5f1 100644 --- a/src/Runner.Worker/StepsRunner.cs +++ b/src/Runner.Worker/StepsRunner.cs @@ -356,28 +356,8 @@ private async Task RunStepAsync(IStep step, CancellationToken jobCancellationTok private void CompleteStep(IStep step, TaskResult? result = null, string resultCode = null) { var executionContext = step.ExecutionContext; - var parentContext = executionContext.Root; - var stepSummaryFilePath = executionContext.GetGitHubContext("step_summary"); - Trace.Info($"Reading step summary data from {stepSummaryFilePath}"); - - var summaryExists = File.Exists(stepSummaryFilePath); - if (summaryExists) - { - Trace.Info($"File exists: {stepSummaryFilePath}"); - - var summaryFileIsEmpty = new FileInfo(stepSummaryFilePath).Length == 0; - if (summaryFileIsEmpty) - { - Trace.Info($"Summary file ({summaryFileIsEmpty}) is empty, skipping attachment upload"); - } - else - { - var stepID = executionContext.Id; - Trace.Info($"Queueing file ({stepSummaryFilePath}) for attachment upload ({stepID})"); - parentContext.QueueAttachFile(ChecksAttachmentType.StepSummary, stepID.ToString(), stepSummaryFilePath); - } - } + CreateSummaryAttachment(step); executionContext.Complete(result, resultCode: resultCode); } @@ -398,5 +378,31 @@ private string CreateStepSummaryFile(IStep step) File.Create(stepSummaryFilePath).Close(); return stepSummaryFilePath; } + + private void CreateSummaryAttachment(IStep step) { + var executionContext = step.ExecutionContext; + var parentContext = executionContext.Root; + var stepSummaryFilePath = executionContext.GetGitHubContext("step_summary"); + var stepID = executionContext.Id; + + Trace.Info($"Reading step summary data from {stepSummaryFilePath}"); + + var summaryExists = File.Exists(stepSummaryFilePath); + if (summaryExists) + { + Trace.Info($"File exists: {stepSummaryFilePath}"); + + var summaryFileIsEmpty = new FileInfo(stepSummaryFilePath).Length == 0; + if (summaryFileIsEmpty) + { + Trace.Info($"Summary file ({summaryFileIsEmpty}) is empty, skipping attachment upload"); + } + else + { + Trace.Info($"Queueing file ({stepSummaryFilePath}) for attachment upload ({stepID})"); + parentContext.QueueAttachFile(ChecksAttachmentType.StepSummary, stepID.ToString(), stepSummaryFilePath); + } + } + } } } \ No newline at end of file From aacf1ba363ddbe481db3e67e153dabc1cdf7fc0a Mon Sep 17 00:00:00 2001 From: Sven Pfleiderer Date: Wed, 26 Jan 2022 15:45:26 -0800 Subject: [PATCH 06/29] Fix indentation --- src/Runner.Worker/StepsRunner.cs | 45 ++++++++++++++++---------------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/src/Runner.Worker/StepsRunner.cs b/src/Runner.Worker/StepsRunner.cs index fc3d41de5f1..c93d3481795 100644 --- a/src/Runner.Worker/StepsRunner.cs +++ b/src/Runner.Worker/StepsRunner.cs @@ -367,8 +367,8 @@ private string CreateStepSummaryFile(IStep step) var stepSummaryDirectory = HostContext.GetDirectory(WellKnownDirectory.StepSummary); if (!Directory.Exists(stepSummaryDirectory)) { - Trace.Info($"Creating step summary directory: {stepSummaryDirectory}"); - Directory.CreateDirectory(stepSummaryDirectory); + Trace.Info($"Creating step summary directory: {stepSummaryDirectory}"); + Directory.CreateDirectory(stepSummaryDirectory); } var stepID = step.ExecutionContext.Id; @@ -379,30 +379,31 @@ private string CreateStepSummaryFile(IStep step) return stepSummaryFilePath; } - private void CreateSummaryAttachment(IStep step) { - var executionContext = step.ExecutionContext; - var parentContext = executionContext.Root; - var stepSummaryFilePath = executionContext.GetGitHubContext("step_summary"); - var stepID = executionContext.Id; - - Trace.Info($"Reading step summary data from {stepSummaryFilePath}"); + private void CreateSummaryAttachment(IStep step) + { + var executionContext = step.ExecutionContext; + var parentContext = executionContext.Root; + var stepSummaryFilePath = executionContext.GetGitHubContext("step_summary"); + var stepID = executionContext.Id; - var summaryExists = File.Exists(stepSummaryFilePath); - if (summaryExists) - { - Trace.Info($"File exists: {stepSummaryFilePath}"); + Trace.Info($"Reading step summary data from {stepSummaryFilePath}"); - var summaryFileIsEmpty = new FileInfo(stepSummaryFilePath).Length == 0; - if (summaryFileIsEmpty) + var summaryExists = File.Exists(stepSummaryFilePath); + if (summaryExists) { - Trace.Info($"Summary file ({summaryFileIsEmpty}) is empty, skipping attachment upload"); - } - else - { - Trace.Info($"Queueing file ({stepSummaryFilePath}) for attachment upload ({stepID})"); - parentContext.QueueAttachFile(ChecksAttachmentType.StepSummary, stepID.ToString(), stepSummaryFilePath); + Trace.Info($"File exists: {stepSummaryFilePath}"); + + var summaryFileIsEmpty = new FileInfo(stepSummaryFilePath).Length == 0; + if (summaryFileIsEmpty) + { + Trace.Info($"Summary file ({summaryFileIsEmpty}) is empty, skipping attachment upload"); + } + else + { + Trace.Info($"Queueing file ({stepSummaryFilePath}) for attachment upload ({stepID})"); + parentContext.QueueAttachFile(ChecksAttachmentType.StepSummary, stepID.ToString(), stepSummaryFilePath); + } } - } } } } \ No newline at end of file From 3ef3cae74d665ae6a41d5a4872bd2d506d5a5155 Mon Sep 17 00:00:00 2001 From: Sven Pfleiderer Date: Wed, 26 Jan 2022 16:23:47 -0800 Subject: [PATCH 07/29] Add (experimental) feature flag support --- src/Runner.Worker/ActionCommandManager.cs | 5 +++++ src/Runner.Worker/StepsRunner.cs | 23 +++++++++++++++++++---- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/src/Runner.Worker/ActionCommandManager.cs b/src/Runner.Worker/ActionCommandManager.cs index 6375db2934a..1ea7806ba49 100644 --- a/src/Runner.Worker/ActionCommandManager.cs +++ b/src/Runner.Worker/ActionCommandManager.cs @@ -191,6 +191,11 @@ internal static bool EnhancedAnnotationsEnabled(IExecutionContext context) { return context.Global.Variables.GetBoolean("DistributedTask.EnhancedAnnotations") ?? false; } + + internal static bool StepSummaryEnabled(IExecutionContext context) + { + return context.Global.Variables.GetBoolean("DistributedTask.StepSummary") ?? false; + } } public interface IActionCommandExtension : IExtension diff --git a/src/Runner.Worker/StepsRunner.cs b/src/Runner.Worker/StepsRunner.cs index c93d3481795..164e796c0fa 100644 --- a/src/Runner.Worker/StepsRunner.cs +++ b/src/Runner.Worker/StepsRunner.cs @@ -104,9 +104,16 @@ public async Task RunAsync(IExecutionContext jobContext) // Set GITHUB_ACTION step.ExecutionContext.SetGitHubContext("action", actionStep.Action.Name); - var stepSummaryFilePath = CreateStepSummaryFile(step); - step.ExecutionContext.SetGitHubContext("step_summary", stepSummaryFilePath); - envContext["GITHUB_STEP_SUMMARY"] = new StringContextData(stepSummaryFilePath); + if (ActionCommandManager.StepSummaryEnabled(step.ExecutionContext)) + { + var stepSummaryFilePath = CreateStepSummaryFile(step); + step.ExecutionContext.SetGitHubContext("step_summary", stepSummaryFilePath); + envContext["GITHUB_STEP_SUMMARY"] = new StringContextData(stepSummaryFilePath); + } + else + { + Trace.Info("Step Summary is disabled; skipping file creation"); + } try { @@ -357,7 +364,15 @@ private void CompleteStep(IStep step, TaskResult? result = null, string resultCo { var executionContext = step.ExecutionContext; - CreateSummaryAttachment(step); + if (ActionCommandManager.StepSummaryEnabled(step.ExecutionContext)) + { + CreateSummaryAttachment(step); + } + else + { + Trace.Info("Step Summary is disabled; skipping file creation"); + + } executionContext.Complete(result, resultCode: resultCode); } From d9d096342d0073fc45fca2e8657ce8dd49c10f14 Mon Sep 17 00:00:00 2001 From: Rob Herley Date: Mon, 31 Jan 2022 15:35:28 -0500 Subject: [PATCH 08/29] reorganize summary upload determination logic --- src/Runner.Worker/StepsRunner.cs | 63 ++++++++++++++++++-------------- 1 file changed, 36 insertions(+), 27 deletions(-) diff --git a/src/Runner.Worker/StepsRunner.cs b/src/Runner.Worker/StepsRunner.cs index 164e796c0fa..3f85634e158 100644 --- a/src/Runner.Worker/StepsRunner.cs +++ b/src/Runner.Worker/StepsRunner.cs @@ -364,19 +364,43 @@ private void CompleteStep(IStep step, TaskResult? result = null, string resultCo { var executionContext = step.ExecutionContext; - if (ActionCommandManager.StepSummaryEnabled(step.ExecutionContext)) + if (ShouldUploadAttachment(step)) { - CreateSummaryAttachment(step); - } - else - { - Trace.Info("Step Summary is disabled; skipping file creation"); - + QueueStepSummaryUpload(step); } executionContext.Complete(result, resultCode: resultCode); } + private bool ShouldUploadAttachment(IStep step) + { + var executionContext = step.ExecutionContext; + var stepSummaryFilePath = executionContext.GetGitHubContext("step_summary"); + + if(!ActionCommandManager.StepSummaryEnabled(step.ExecutionContext)) + { + Trace.Info("Step Summary is disabled; skipping attachment upload"); + return false; + } + + var summaryExists = File.Exists(stepSummaryFilePath); + if (summaryExists) + { + Trace.Info($"Summary file ({stepSummaryFilePath}) does not exist; skipping attachment upload"); + return false; + } + Trace.Info($"Summary file exists: {stepSummaryFilePath}"); + + var summaryFileIsEmpty = new FileInfo(stepSummaryFilePath).Length == 0; + if (summaryFileIsEmpty) + { + Trace.Info($"Summary file ({stepSummaryFilePath}) is empty; skipping attachment upload"); + return false; + } + + return true; + } + private string CreateStepSummaryFile(IStep step) { var stepSummaryDirectory = HostContext.GetDirectory(WellKnownDirectory.StepSummary); @@ -394,31 +418,16 @@ private string CreateStepSummaryFile(IStep step) return stepSummaryFilePath; } - private void CreateSummaryAttachment(IStep step) + private void QueueStepSummaryUpload(IStep step) { var executionContext = step.ExecutionContext; var parentContext = executionContext.Root; var stepSummaryFilePath = executionContext.GetGitHubContext("step_summary"); - var stepID = executionContext.Id; - - Trace.Info($"Reading step summary data from {stepSummaryFilePath}"); - - var summaryExists = File.Exists(stepSummaryFilePath); - if (summaryExists) - { - Trace.Info($"File exists: {stepSummaryFilePath}"); + var stepID = step.ExecutionContext.Id; + var attachmentName = stepID.ToString(); - var summaryFileIsEmpty = new FileInfo(stepSummaryFilePath).Length == 0; - if (summaryFileIsEmpty) - { - Trace.Info($"Summary file ({summaryFileIsEmpty}) is empty, skipping attachment upload"); - } - else - { - Trace.Info($"Queueing file ({stepSummaryFilePath}) for attachment upload ({stepID})"); - parentContext.QueueAttachFile(ChecksAttachmentType.StepSummary, stepID.ToString(), stepSummaryFilePath); - } - } + Trace.Info($"Queueing file ({stepSummaryFilePath}) for attachment upload ({attachmentName})"); + parentContext.QueueAttachFile(ChecksAttachmentType.StepSummary, attachmentName, stepSummaryFilePath); } } } \ No newline at end of file From 901a4060e7840f47da5e40bc6dae0dd8379b2f71 Mon Sep 17 00:00:00 2001 From: Rob Herley Date: Tue, 1 Feb 2022 15:41:44 +0000 Subject: [PATCH 09/29] file i/o exception handling + pr feedback --- src/Runner.Worker/StepsRunner.cs | 63 ++++++++++++++++++++++++++------ 1 file changed, 51 insertions(+), 12 deletions(-) diff --git a/src/Runner.Worker/StepsRunner.cs b/src/Runner.Worker/StepsRunner.cs index 3f85634e158..3f7fb29e2e7 100644 --- a/src/Runner.Worker/StepsRunner.cs +++ b/src/Runner.Worker/StepsRunner.cs @@ -107,8 +107,11 @@ public async Task RunAsync(IExecutionContext jobContext) if (ActionCommandManager.StepSummaryEnabled(step.ExecutionContext)) { var stepSummaryFilePath = CreateStepSummaryFile(step); - step.ExecutionContext.SetGitHubContext("step_summary", stepSummaryFilePath); - envContext["GITHUB_STEP_SUMMARY"] = new StringContextData(stepSummaryFilePath); + if (!String.IsNullOrEmpty(stepSummaryFilePath)) + { + step.ExecutionContext.SetGitHubContext("step_summary", stepSummaryFilePath); + envContext["GITHUB_STEP_SUMMARY"] = new StringContextData(stepSummaryFilePath); + } } else { @@ -377,24 +380,29 @@ private bool ShouldUploadAttachment(IStep step) var executionContext = step.ExecutionContext; var stepSummaryFilePath = executionContext.GetGitHubContext("step_summary"); - if(!ActionCommandManager.StepSummaryEnabled(step.ExecutionContext)) + if (!ActionCommandManager.StepSummaryEnabled(step.ExecutionContext)) { Trace.Info("Step Summary is disabled; skipping attachment upload"); return false; } - var summaryExists = File.Exists(stepSummaryFilePath); - if (summaryExists) + if (String.IsNullOrEmpty(stepSummaryFilePath)) + { + Trace.Info("Step Summary path is empty; skipping attachment upload"); + return false; + } + + if (File.Exists(stepSummaryFilePath)) { - Trace.Info($"Summary file ({stepSummaryFilePath}) does not exist; skipping attachment upload"); + Trace.Info($"Step Summary file ({stepSummaryFilePath}) does not exist; skipping attachment upload"); return false; } - Trace.Info($"Summary file exists: {stepSummaryFilePath}"); - var summaryFileIsEmpty = new FileInfo(stepSummaryFilePath).Length == 0; - if (summaryFileIsEmpty) + Trace.Info($"Step Summary file exists: {stepSummaryFilePath}"); + + if (new FileInfo(stepSummaryFilePath).Length == 0) { - Trace.Info($"Summary file ({stepSummaryFilePath}) is empty; skipping attachment upload"); + Trace.Info($"Step Summary file ({stepSummaryFilePath}) is empty; skipping attachment upload"); return false; } @@ -407,14 +415,45 @@ private string CreateStepSummaryFile(IStep step) if (!Directory.Exists(stepSummaryDirectory)) { Trace.Info($"Creating step summary directory: {stepSummaryDirectory}"); - Directory.CreateDirectory(stepSummaryDirectory); + try + { + Directory.CreateDirectory(stepSummaryDirectory); + } + catch (UnauthorizedAccessException e) + { + Trace.Error($"Permission is required to create step summary directory: {stepSummaryDirectory}"); + Trace.Error(e); + return null; + } + catch (Exception e) + { + Trace.Error($"Unable to create step summary directory: {stepSummaryDirectory}"); + Trace.Error(e); + return null; + } } var stepID = step.ExecutionContext.Id; var stepSummaryFilePath = Path.Combine(stepSummaryDirectory, $"{stepID}.md"); Trace.Info($"Creating step summary file: {stepSummaryFilePath}"); - File.Create(stepSummaryFilePath).Close(); + try + { + File.Create(stepSummaryFilePath).Close(); + } + catch (UnauthorizedAccessException e) + { + Trace.Error($"Permission is required to create step summary file: {stepSummaryFilePath}"); + Trace.Error(e); + return null; + } + catch (Exception e) + { + Trace.Error($"Unable to create step summary file: {stepSummaryFilePath}"); + Trace.Error(e); + return null; + } + return stepSummaryFilePath; } From 9572ebf81efb34fa5d82db3524b06eeb2a052bca Mon Sep 17 00:00:00 2001 From: Sven Pfleiderer Date: Tue, 1 Feb 2022 23:29:50 +0000 Subject: [PATCH 10/29] Revert changes for now to reintroduce them later --- src/Runner.Common/Constants.cs | 2 - src/Runner.Common/HostContext.cs | 6 -- src/Runner.Worker/StepsRunner.cs | 116 +------------------------------ 3 files changed, 1 insertion(+), 123 deletions(-) diff --git a/src/Runner.Common/Constants.cs b/src/Runner.Common/Constants.cs index 23a6a10c178..829c8223edf 100644 --- a/src/Runner.Common/Constants.cs +++ b/src/Runner.Common/Constants.cs @@ -9,7 +9,6 @@ public enum WellKnownDirectory Externals, Root, Actions, - StepSummary, Temp, Tools, Update, @@ -198,7 +197,6 @@ public static class Path public static readonly string DiagDirectory = "_diag"; public static readonly string ExternalsDirectory = "externals"; public static readonly string RunnerDiagnosticLogPrefix = "Runner_"; - public static readonly string StepSummaryDirectory = "_step_summary"; public static readonly string TempDirectory = "_temp"; public static readonly string ToolDirectory = "_tool"; public static readonly string UpdateDirectory = "_update"; diff --git a/src/Runner.Common/HostContext.cs b/src/Runner.Common/HostContext.cs index df6e36bebe7..643c61c8125 100644 --- a/src/Runner.Common/HostContext.cs +++ b/src/Runner.Common/HostContext.cs @@ -243,12 +243,6 @@ public string GetDirectory(WellKnownDirectory directory) path = new DirectoryInfo(GetDirectory(WellKnownDirectory.Bin)).Parent.FullName; break; - case WellKnownDirectory.StepSummary: - path = Path.Combine( - GetDirectory(WellKnownDirectory.Temp), - Constants.Path.StepSummaryDirectory); - break; - case WellKnownDirectory.Temp: path = Path.Combine( GetDirectory(WellKnownDirectory.Work), diff --git a/src/Runner.Worker/StepsRunner.cs b/src/Runner.Worker/StepsRunner.cs index 3f7fb29e2e7..7b7bc971670 100644 --- a/src/Runner.Worker/StepsRunner.cs +++ b/src/Runner.Worker/StepsRunner.cs @@ -1,6 +1,5 @@ using System; using System.Collections.Generic; -using System.IO; using System.Text; using System.Threading; using System.Threading.Tasks; @@ -104,20 +103,6 @@ public async Task RunAsync(IExecutionContext jobContext) // Set GITHUB_ACTION step.ExecutionContext.SetGitHubContext("action", actionStep.Action.Name); - if (ActionCommandManager.StepSummaryEnabled(step.ExecutionContext)) - { - var stepSummaryFilePath = CreateStepSummaryFile(step); - if (!String.IsNullOrEmpty(stepSummaryFilePath)) - { - step.ExecutionContext.SetGitHubContext("step_summary", stepSummaryFilePath); - envContext["GITHUB_STEP_SUMMARY"] = new StringContextData(stepSummaryFilePath); - } - } - else - { - Trace.Info("Step Summary is disabled; skipping file creation"); - } - try { // Evaluate and merge step env @@ -367,106 +352,7 @@ private void CompleteStep(IStep step, TaskResult? result = null, string resultCo { var executionContext = step.ExecutionContext; - if (ShouldUploadAttachment(step)) - { - QueueStepSummaryUpload(step); - } - executionContext.Complete(result, resultCode: resultCode); } - - private bool ShouldUploadAttachment(IStep step) - { - var executionContext = step.ExecutionContext; - var stepSummaryFilePath = executionContext.GetGitHubContext("step_summary"); - - if (!ActionCommandManager.StepSummaryEnabled(step.ExecutionContext)) - { - Trace.Info("Step Summary is disabled; skipping attachment upload"); - return false; - } - - if (String.IsNullOrEmpty(stepSummaryFilePath)) - { - Trace.Info("Step Summary path is empty; skipping attachment upload"); - return false; - } - - if (File.Exists(stepSummaryFilePath)) - { - Trace.Info($"Step Summary file ({stepSummaryFilePath}) does not exist; skipping attachment upload"); - return false; - } - - Trace.Info($"Step Summary file exists: {stepSummaryFilePath}"); - - if (new FileInfo(stepSummaryFilePath).Length == 0) - { - Trace.Info($"Step Summary file ({stepSummaryFilePath}) is empty; skipping attachment upload"); - return false; - } - - return true; - } - - private string CreateStepSummaryFile(IStep step) - { - var stepSummaryDirectory = HostContext.GetDirectory(WellKnownDirectory.StepSummary); - if (!Directory.Exists(stepSummaryDirectory)) - { - Trace.Info($"Creating step summary directory: {stepSummaryDirectory}"); - try - { - Directory.CreateDirectory(stepSummaryDirectory); - } - catch (UnauthorizedAccessException e) - { - Trace.Error($"Permission is required to create step summary directory: {stepSummaryDirectory}"); - Trace.Error(e); - return null; - } - catch (Exception e) - { - Trace.Error($"Unable to create step summary directory: {stepSummaryDirectory}"); - Trace.Error(e); - return null; - } - } - - var stepID = step.ExecutionContext.Id; - var stepSummaryFilePath = Path.Combine(stepSummaryDirectory, $"{stepID}.md"); - - Trace.Info($"Creating step summary file: {stepSummaryFilePath}"); - try - { - File.Create(stepSummaryFilePath).Close(); - } - catch (UnauthorizedAccessException e) - { - Trace.Error($"Permission is required to create step summary file: {stepSummaryFilePath}"); - Trace.Error(e); - return null; - } - catch (Exception e) - { - Trace.Error($"Unable to create step summary file: {stepSummaryFilePath}"); - Trace.Error(e); - return null; - } - - return stepSummaryFilePath; - } - - private void QueueStepSummaryUpload(IStep step) - { - var executionContext = step.ExecutionContext; - var parentContext = executionContext.Root; - var stepSummaryFilePath = executionContext.GetGitHubContext("step_summary"); - var stepID = step.ExecutionContext.Id; - var attachmentName = stepID.ToString(); - - Trace.Info($"Queueing file ({stepSummaryFilePath}) for attachment upload ({attachmentName})"); - parentContext.QueueAttachFile(ChecksAttachmentType.StepSummary, attachmentName, stepSummaryFilePath); - } } -} \ No newline at end of file +} From 44ab8711fa290d037faab20cd6061c9cd21e6fdb Mon Sep 17 00:00:00 2001 From: Sven Pfleiderer Date: Tue, 1 Feb 2022 23:57:13 +0000 Subject: [PATCH 11/29] Add skeleton SetStepSummaryCommand --- src/Runner.Common/ExtensionManager.cs | 1 + src/Runner.Worker/FileCommandManager.cs | 16 ++++++++++++++++ src/Runner.Worker/GitHubContext.cs | 1 + 3 files changed, 18 insertions(+) diff --git a/src/Runner.Common/ExtensionManager.cs b/src/Runner.Common/ExtensionManager.cs index a432d6d3988..daeebf7126c 100644 --- a/src/Runner.Common/ExtensionManager.cs +++ b/src/Runner.Common/ExtensionManager.cs @@ -60,6 +60,7 @@ private List LoadExtensions() where T : class, IExtension case "GitHub.Runner.Worker.IFileCommandExtension": Add(extensions, "GitHub.Runner.Worker.AddPathFileCommand, Runner.Worker"); Add(extensions, "GitHub.Runner.Worker.SetEnvFileCommand, Runner.Worker"); + Add(extensions, "GitHub.Runner.Worker.SetStepSummaryCommand, Runner.Worker"); break; case "GitHub.Runner.Listener.Check.ICheckExtension": Add(extensions, "GitHub.Runner.Listener.Check.InternetCheck, Runner.Listener"); diff --git a/src/Runner.Worker/FileCommandManager.cs b/src/Runner.Worker/FileCommandManager.cs index aea76f10a06..69fe1f12daa 100644 --- a/src/Runner.Worker/FileCommandManager.cs +++ b/src/Runner.Worker/FileCommandManager.cs @@ -259,4 +259,20 @@ private static string ReadLine( return text.Substring(originalIndex, lfIndex - originalIndex); } } + + public sealed class SetStepSummaryCommand: RunnerService, IFileCommandExtension + { + public string ContextName => "step_summary"; + public string FilePrefix => "step_summary_"; + + public Type ExtensionType => typeof(IFileCommandExtension); + + public void ProcessCommand(IExecutionContext context, string filePath, ContainerInfo container) + { + if (File.Exists(filePath)) + { + Trace.Info($"Submitting step summary content from file {filePath}"); + } + } + } } diff --git a/src/Runner.Worker/GitHubContext.cs b/src/Runner.Worker/GitHubContext.cs index 589c17720b5..80f47b3074a 100644 --- a/src/Runner.Worker/GitHubContext.cs +++ b/src/Runner.Worker/GitHubContext.cs @@ -34,6 +34,7 @@ public sealed class GitHubContext : DictionaryContextData, IEnvironmentContextDa "run_number", "server_url", "sha", + "step_summary", "workflow", "workspace", }; From 6b129148ed6ce1727c055e8a0349de532ca0f801 Mon Sep 17 00:00:00 2001 From: Sven Pfleiderer Date: Wed, 2 Feb 2022 00:08:45 +0000 Subject: [PATCH 12/29] Update step summary feature flag name --- src/Runner.Worker/ActionCommandManager.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Runner.Worker/ActionCommandManager.cs b/src/Runner.Worker/ActionCommandManager.cs index 1ea7806ba49..1c48109aa32 100644 --- a/src/Runner.Worker/ActionCommandManager.cs +++ b/src/Runner.Worker/ActionCommandManager.cs @@ -194,7 +194,7 @@ internal static bool EnhancedAnnotationsEnabled(IExecutionContext context) internal static bool StepSummaryEnabled(IExecutionContext context) { - return context.Global.Variables.GetBoolean("DistributedTask.StepSummary") ?? false; + return context.Global.Variables.GetBoolean("DistributedTask.UploadStepSummary") ?? false; } } From a39bbf247b38bbaa5a13a5f50c3f66f16a52d45d Mon Sep 17 00:00:00 2001 From: Sven Pfleiderer Date: Wed, 2 Feb 2022 00:11:52 +0000 Subject: [PATCH 13/29] Port ShouldUploadAttachment from previous iteration --- src/Runner.Worker/FileCommandManager.cs | 33 ++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/src/Runner.Worker/FileCommandManager.cs b/src/Runner.Worker/FileCommandManager.cs index 69fe1f12daa..33564efa42a 100644 --- a/src/Runner.Worker/FileCommandManager.cs +++ b/src/Runner.Worker/FileCommandManager.cs @@ -269,10 +269,41 @@ public sealed class SetStepSummaryCommand: RunnerService, IFileCommandExtension public void ProcessCommand(IExecutionContext context, string filePath, ContainerInfo container) { - if (File.Exists(filePath)) + if (ShouldUploadAttachment(context, filePath)) { Trace.Info($"Submitting step summary content from file {filePath}"); } } + + private bool ShouldUploadAttachment(IExecutionContext context, string filePath) + { + if (!ActionCommandManager.StepSummaryEnabled(context)) + { + Trace.Info("Step Summary is disabled; skipping attachment upload"); + return false; + } + + if (String.IsNullOrEmpty(filePath)) + { + Trace.Info("Step Summary path is empty; skipping attachment upload"); + return false; + } + + if (!File.Exists(filePath)) + { + Trace.Info($"Step Summary file ({filePath}) does not exist; skipping attachment upload"); + return false; + } + + Trace.Info($"Step Summary file exists: {filePath}"); + + if (new FileInfo(filePath).Length == 0) + { + Trace.Info($"Step Summary file ({filePath}) is empty; skipping attachment upload"); + return false; + } + + return true; + } } } From 5757734524d695a4e5894cd3479711a4703c6cb7 Mon Sep 17 00:00:00 2001 From: Sven Pfleiderer Date: Wed, 2 Feb 2022 00:21:16 +0000 Subject: [PATCH 14/29] Port QueueStepSummaryUpload from previous iteration --- src/Runner.Worker/FileCommandManager.cs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/Runner.Worker/FileCommandManager.cs b/src/Runner.Worker/FileCommandManager.cs index 33564efa42a..5a485e38282 100644 --- a/src/Runner.Worker/FileCommandManager.cs +++ b/src/Runner.Worker/FileCommandManager.cs @@ -260,7 +260,7 @@ private static string ReadLine( } } - public sealed class SetStepSummaryCommand: RunnerService, IFileCommandExtension + public sealed class SetStepSummaryCommand : RunnerService, IFileCommandExtension { public string ContextName => "step_summary"; public string FilePrefix => "step_summary_"; @@ -271,7 +271,8 @@ public void ProcessCommand(IExecutionContext context, string filePath, Container { if (ShouldUploadAttachment(context, filePath)) { - Trace.Info($"Submitting step summary content from file {filePath}"); + Trace.Info($"Submitting step summary content from file {filePath}, container: {container}"); + QueueStepSummaryUpload(context, filePath); } } @@ -305,5 +306,15 @@ private bool ShouldUploadAttachment(IExecutionContext context, string filePath) return true; } + + private void QueueStepSummaryUpload(IExecutionContext context, string filePath) + { + var parentContext = context.Root; + var stepID = context.Id; + var attachmentName = stepID.ToString(); + + Trace.Info($"Queueing file ({filePath}) for attachment upload ({attachmentName})"); + parentContext.QueueAttachFile(ChecksAttachmentType.StepSummary, attachmentName, filePath); + } } } From d1e2740ee2d6ff2ad927b8c895e4538402cf9e77 Mon Sep 17 00:00:00 2001 From: Sven Pfleiderer Date: Wed, 2 Feb 2022 00:29:31 +0000 Subject: [PATCH 15/29] Improve exception handling when uploading attachment --- src/Runner.Worker/FileCommandManager.cs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/Runner.Worker/FileCommandManager.cs b/src/Runner.Worker/FileCommandManager.cs index 5a485e38282..dbc5cfba62e 100644 --- a/src/Runner.Worker/FileCommandManager.cs +++ b/src/Runner.Worker/FileCommandManager.cs @@ -314,7 +314,14 @@ private void QueueStepSummaryUpload(IExecutionContext context, string filePath) var attachmentName = stepID.ToString(); Trace.Info($"Queueing file ({filePath}) for attachment upload ({attachmentName})"); - parentContext.QueueAttachFile(ChecksAttachmentType.StepSummary, attachmentName, filePath); + try + { + parentContext.QueueAttachFile(ChecksAttachmentType.StepSummary, attachmentName, filePath); + } + catch (Exception e) + { + Trace.Error($"Error while trying to enqueue file upload for file ({filePath}): {e}"); + } } } } From 4dddab6444e13e9f2e105c1aba1dfb8a3c023f6e Mon Sep 17 00:00:00 2001 From: Sven Pfleiderer Date: Wed, 2 Feb 2022 19:30:14 +0000 Subject: [PATCH 16/29] Add some minor logging improvements --- src/Runner.Worker/FileCommandManager.cs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/Runner.Worker/FileCommandManager.cs b/src/Runner.Worker/FileCommandManager.cs index dbc5cfba62e..3c06e927e3d 100644 --- a/src/Runner.Worker/FileCommandManager.cs +++ b/src/Runner.Worker/FileCommandManager.cs @@ -296,14 +296,15 @@ private bool ShouldUploadAttachment(IExecutionContext context, string filePath) return false; } - Trace.Info($"Step Summary file exists: {filePath}"); - - if (new FileInfo(filePath).Length == 0) + var fileSize = new FileInfo(filePath).Length; + if (fileSize == 0) { Trace.Info($"Step Summary file ({filePath}) is empty; skipping attachment upload"); return false; } + Trace.Info($"Step Summary file exists: {filePath} and has a file size of {fileSize} bytes"); + return true; } From 10935cdac64e3776f27f61c4ceb06a0b43266bb6 Mon Sep 17 00:00:00 2001 From: Sven Pfleiderer Date: Wed, 2 Feb 2022 21:50:14 +0000 Subject: [PATCH 17/29] Refuse to upload files larger than 128k --- src/Runner.Worker/FileCommandManager.cs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/Runner.Worker/FileCommandManager.cs b/src/Runner.Worker/FileCommandManager.cs index 3c06e927e3d..99a2be33f68 100644 --- a/src/Runner.Worker/FileCommandManager.cs +++ b/src/Runner.Worker/FileCommandManager.cs @@ -265,6 +265,8 @@ public sealed class SetStepSummaryCommand : RunnerService, IFileCommandExtension public string ContextName => "step_summary"; public string FilePrefix => "step_summary_"; + private const int AttachmentSizeLimit = 128 * 1024; + public Type ExtensionType => typeof(IFileCommandExtension); public void ProcessCommand(IExecutionContext context, string filePath, ContainerInfo container) @@ -303,6 +305,14 @@ private bool ShouldUploadAttachment(IExecutionContext context, string filePath) return false; } + if (fileSize > AttachmentSizeLimit) + { + context.Error($"$GITHUB_STEP_SUMMARY supports content up a size of {AttachmentSizeLimit/1024}k got {fileSize/1024}k"); + Trace.Info($"Step Summary file ({filePath}) is too large ({fileSize} bytes); skipping attachment upload"); + return false; + + } + Trace.Info($"Step Summary file exists: {filePath} and has a file size of {fileSize} bytes"); return true; From 51c110adc675f87c1b34a19804b866e03413d216 Mon Sep 17 00:00:00 2001 From: Sven Pfleiderer Date: Wed, 2 Feb 2022 22:34:58 +0000 Subject: [PATCH 18/29] Implement secrets scrubbing --- src/Runner.Worker/FileCommandManager.cs | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/src/Runner.Worker/FileCommandManager.cs b/src/Runner.Worker/FileCommandManager.cs index 99a2be33f68..e0dafba4029 100644 --- a/src/Runner.Worker/FileCommandManager.cs +++ b/src/Runner.Worker/FileCommandManager.cs @@ -274,7 +274,12 @@ public void ProcessCommand(IExecutionContext context, string filePath, Container if (ShouldUploadAttachment(context, filePath)) { Trace.Info($"Submitting step summary content from file {filePath}, container: {container}"); - QueueStepSummaryUpload(context, filePath); + var scrubbedFilePath = ScrubStepSummaryFileSecrets(filePath); + + File.Copy(filePath, Path.Combine("/tmp", Path.GetFileName(filePath))); + File.Copy(scrubbedFilePath, Path.Combine("/tmp", Path.GetFileName(scrubbedFilePath))); + + QueueStepSummaryUpload(context, scrubbedFilePath); } } @@ -318,6 +323,24 @@ private bool ShouldUploadAttachment(IExecutionContext context, string filePath) return true; } + private string ScrubStepSummaryFileSecrets(string filePath) + { + var scrubbedFilePath = filePath + "-scrubbed"; + + using (var streamReader = new StreamReader(filePath)) + using (var streamWriter = new StreamWriter(scrubbedFilePath)) + { + string line; + while ((line = streamReader.ReadLine()) != null) + { + var maskedLine = HostContext.SecretMasker.MaskSecrets(line); + streamWriter.WriteLine(maskedLine); + } + } + + return scrubbedFilePath; + } + private void QueueStepSummaryUpload(IExecutionContext context, string filePath) { var parentContext = context.Root; From 88f916b694aa0ef8a7b995b81a56a5caf0cbfd68 Mon Sep 17 00:00:00 2001 From: Sven Pfleiderer Date: Wed, 2 Feb 2022 22:36:20 +0000 Subject: [PATCH 19/29] Add TODO comment to remove debugging temp files --- src/Runner.Worker/FileCommandManager.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Runner.Worker/FileCommandManager.cs b/src/Runner.Worker/FileCommandManager.cs index e0dafba4029..18537badf9b 100644 --- a/src/Runner.Worker/FileCommandManager.cs +++ b/src/Runner.Worker/FileCommandManager.cs @@ -276,6 +276,7 @@ public void ProcessCommand(IExecutionContext context, string filePath, Container Trace.Info($"Submitting step summary content from file {filePath}, container: {container}"); var scrubbedFilePath = ScrubStepSummaryFileSecrets(filePath); + // TODO: For debugging only; remove before opening PR File.Copy(filePath, Path.Combine("/tmp", Path.GetFileName(filePath))); File.Copy(scrubbedFilePath, Path.Combine("/tmp", Path.GetFileName(scrubbedFilePath))); From c292c4dadb35e55f3172cb541188b5fcbd08a2b7 Mon Sep 17 00:00:00 2001 From: Sven Pfleiderer Date: Thu, 3 Feb 2022 01:11:02 +0000 Subject: [PATCH 20/29] Add first tests --- src/Runner.Worker/FileCommandManager.cs | 7 +- src/Test/L0/Worker/SetStepSummaryCommandL0.cs | 209 ++++++++++++++++++ 2 files changed, 210 insertions(+), 6 deletions(-) create mode 100644 src/Test/L0/Worker/SetStepSummaryCommandL0.cs diff --git a/src/Runner.Worker/FileCommandManager.cs b/src/Runner.Worker/FileCommandManager.cs index 18537badf9b..868e5fe84df 100644 --- a/src/Runner.Worker/FileCommandManager.cs +++ b/src/Runner.Worker/FileCommandManager.cs @@ -276,10 +276,6 @@ public void ProcessCommand(IExecutionContext context, string filePath, Container Trace.Info($"Submitting step summary content from file {filePath}, container: {container}"); var scrubbedFilePath = ScrubStepSummaryFileSecrets(filePath); - // TODO: For debugging only; remove before opening PR - File.Copy(filePath, Path.Combine("/tmp", Path.GetFileName(filePath))); - File.Copy(scrubbedFilePath, Path.Combine("/tmp", Path.GetFileName(scrubbedFilePath))); - QueueStepSummaryUpload(context, scrubbedFilePath); } } @@ -344,14 +340,13 @@ private string ScrubStepSummaryFileSecrets(string filePath) private void QueueStepSummaryUpload(IExecutionContext context, string filePath) { - var parentContext = context.Root; var stepID = context.Id; var attachmentName = stepID.ToString(); Trace.Info($"Queueing file ({filePath}) for attachment upload ({attachmentName})"); try { - parentContext.QueueAttachFile(ChecksAttachmentType.StepSummary, attachmentName, filePath); + context.QueueAttachFile(ChecksAttachmentType.StepSummary, attachmentName, filePath); } catch (Exception e) { diff --git a/src/Test/L0/Worker/SetStepSummaryCommandL0.cs b/src/Test/L0/Worker/SetStepSummaryCommandL0.cs new file mode 100644 index 00000000000..98111437723 --- /dev/null +++ b/src/Test/L0/Worker/SetStepSummaryCommandL0.cs @@ -0,0 +1,209 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Text; +using System.Runtime.CompilerServices; +using GitHub.Runner.Common.Util; +using GitHub.Runner.Sdk; +using GitHub.Runner.Worker; +using Moq; +using Xunit; +using DTWebApi = GitHub.DistributedTask.WebApi; +using GitHub.DistributedTask.WebApi; + +namespace GitHub.Runner.Common.Tests.Worker +{ + public sealed class SetStepSummaryCommandL0 + { + private Mock _executionContext; + private List> _issues; + private Variables _variables; + private string _rootDirectory; + private SetStepSummaryCommand _setStepCommand; + private ITraceWriter _trace; + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void SetStepSummaryCommand_FeatureDisabled() + { + using (var hostContext = Setup(featureFlagState: "false")) + { + var stepSummaryFile = Path.Combine(_rootDirectory, "feature-off"); + + _executionContext.Verify(e => e.QueueAttachFile(ChecksAttachmentType.StepSummary, It.IsAny(), It.IsAny()), Times.Never()); + + Assert.Equal(0, _issues.Count); + } + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void SetStepSummaryCommand_FileNull() + { + using (var hostContext = Setup()) + { + _setStepCommand.ProcessCommand(_executionContext.Object, null, null); + + _executionContext.Verify(e => e.QueueAttachFile(ChecksAttachmentType.StepSummary, It.IsAny(), It.IsAny()), Times.Never()); + Assert.Equal(0, _issues.Count); + } + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void SetStepSummaryCommand_DirectoryNotFound() + { + using (var hostContext = Setup()) + { + var stepSummaryFile = Path.Combine(_rootDirectory, "directory-not-found", "env"); + + _setStepCommand.ProcessCommand(_executionContext.Object, stepSummaryFile, null); + + _executionContext.Verify(e => e.QueueAttachFile(ChecksAttachmentType.StepSummary, It.IsAny(), It.IsAny()), Times.Never()); + Assert.Equal(0, _issues.Count); + } + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void SetStepSummaryCommand_FileNotFound() + { + using (var hostContext = Setup()) + { + var stepSummaryFile = Path.Combine(_rootDirectory, "file-not-found"); + + _setStepCommand.ProcessCommand(_executionContext.Object, stepSummaryFile, null); + + _executionContext.Verify(e => e.QueueAttachFile(ChecksAttachmentType.StepSummary, It.IsAny(), It.IsAny()), Times.Never()); + Assert.Equal(0, _issues.Count); + } + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void SetStepSummaryCommand_EmptyFile() + { + using (var hostContext = Setup()) + { + var stepSummaryFile = Path.Combine(_rootDirectory, "empty-file"); + File.Create(stepSummaryFile).Dispose(); + + _setStepCommand.ProcessCommand(_executionContext.Object, stepSummaryFile, null); + + _executionContext.Verify(e => e.QueueAttachFile(ChecksAttachmentType.StepSummary, It.IsAny(), It.IsAny()), Times.Never()); + Assert.Equal(0, _issues.Count); + } + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void SetStepSummaryCommand_LargeFile() + { + using (var hostContext = Setup()) + { + var stepSummaryFile = Path.Combine(_rootDirectory, "empty-file"); + File.WriteAllBytes(stepSummaryFile, new byte[128 * 1024 + 1]); + + _setStepCommand.ProcessCommand(_executionContext.Object, stepSummaryFile, null); + + _executionContext.Verify(e => e.QueueAttachFile(ChecksAttachmentType.StepSummary, It.IsAny(), It.IsAny()), Times.Never()); + Assert.Equal(1, _issues.Count); + } + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void SetStepSummaryCommand_Simple() + { + using (var hostContext = Setup()) + { + var stepSummaryFile = Path.Combine(_rootDirectory, "simple"); + var content = new List + { + "# This is some markdown content", + "", + "## This is more markdown content", + }; + WriteContent(stepSummaryFile, content); + + _setStepCommand.ProcessCommand(_executionContext.Object, stepSummaryFile, null); + + _executionContext.Verify(e => e.QueueAttachFile(ChecksAttachmentType.StepSummary, _executionContext.Object.Id.ToString(), stepSummaryFile + "-scrubbed"), Times.Once()); + Assert.Equal(0, _issues.Count); + } + } + + private void WriteContent( + string path, + List content, + string newline = null) + { + if (string.IsNullOrEmpty(newline)) + { + newline = Environment.NewLine; + } + + var encoding = new UTF8Encoding(true); // Emit BOM + var contentStr = string.Join(newline, content); + File.WriteAllText(path, contentStr, encoding); + } + + private TestHostContext Setup([CallerMemberName] string name = "", string featureFlagState = "true") + { + _issues = new List>(); + + var hostContext = new TestHostContext(this, name); + + // Trace + _trace = hostContext.GetTrace(); + + _variables = new Variables(hostContext, new Dictionary + { + { "MySecretName", new VariableValue("My secret value", true) }, + { "DistributedTask.UploadStepSummary", featureFlagState }, + }); + + // Directory for test data + var workDirectory = hostContext.GetDirectory(WellKnownDirectory.Work); + ArgUtil.NotNullOrEmpty(workDirectory, nameof(workDirectory)); + Directory.CreateDirectory(workDirectory); + _rootDirectory = Path.Combine(workDirectory, nameof(SetStepSummaryCommandL0)); + Directory.CreateDirectory(_rootDirectory); + + // Execution context + _executionContext = new Mock(); + _executionContext.Setup(x => x.Global) + .Returns(new GlobalContext + { + EnvironmentVariables = new Dictionary(VarUtil.EnvironmentVariableKeyComparer), + WriteDebug = true, + Variables = _variables, + }); + _executionContext.Setup(x => x.AddIssue(It.IsAny(), It.IsAny())) + .Callback((DTWebApi.Issue issue, string logMessage) => + { + _issues.Add(new Tuple(issue, logMessage)); + var message = !string.IsNullOrEmpty(logMessage) ? logMessage : issue.Message; + _trace.Info($"Issue '{issue.Type}': {message}"); + }); + _executionContext.Setup(x => x.Write(It.IsAny(), It.IsAny())) + .Callback((string tag, string message) => + { + _trace.Info($"{tag}{message}"); + }); + + // SetStepSummaryCommand + _setStepCommand = new SetStepSummaryCommand(); + _setStepCommand.Initialize(hostContext); + + return hostContext; + } + } +} From cad400b1231f48b8b1755d6eb59b321c8bd93a62 Mon Sep 17 00:00:00 2001 From: Sven Pfleiderer Date: Thu, 3 Feb 2022 01:30:34 +0000 Subject: [PATCH 21/29] Add test for secret masking --- src/Test/L0/Worker/SetStepSummaryCommandL0.cs | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/Test/L0/Worker/SetStepSummaryCommandL0.cs b/src/Test/L0/Worker/SetStepSummaryCommandL0.cs index 98111437723..8cb1c84a90f 100644 --- a/src/Test/L0/Worker/SetStepSummaryCommandL0.cs +++ b/src/Test/L0/Worker/SetStepSummaryCommandL0.cs @@ -140,6 +140,38 @@ public void SetStepSummaryCommand_Simple() } } + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void SetStepSummaryCommand_ScrubSecrets() + { + using (var hostContext = Setup()) + { + // configure secretmasker to actually mask secrets + hostContext.SecretMasker.AddRegex("Password=.*"); + hostContext.SecretMasker.AddRegex("ghs_.*"); + + var stepSummaryFile = Path.Combine(_rootDirectory, "simple"); + var scrubbedFile = stepSummaryFile + "-scrubbed"; + var content = new List + { + "# Password=ThisIsMySecretPassword!", + "", + "# GITHUB_TOKEN ghs_verysecuretoken", + }; + WriteContent(stepSummaryFile, content); + + _setStepCommand.ProcessCommand(_executionContext.Object, stepSummaryFile, null); + + var scrubbedFileContents = File.ReadAllText(scrubbedFile); + Assert.DoesNotContain("ThisIsMySecretPassword!", scrubbedFileContents); + Assert.DoesNotContain("ghs_verysecuretoken", scrubbedFileContents); + + _executionContext.Verify(e => e.QueueAttachFile(ChecksAttachmentType.StepSummary, _executionContext.Object.Id.ToString(), scrubbedFile), Times.Once()); + Assert.Equal(0, _issues.Count); + } + } + private void WriteContent( string path, List content, From e69dd47384c108d594ad4a8c5830b65a465f98f0 Mon Sep 17 00:00:00 2001 From: Sven Pfleiderer Date: Thu, 3 Feb 2022 19:01:52 +0000 Subject: [PATCH 22/29] Add some naming/style fixes suggested in feedback --- src/Runner.Worker/FileCommandManager.cs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/Runner.Worker/FileCommandManager.cs b/src/Runner.Worker/FileCommandManager.cs index 868e5fe84df..e087cc203da 100644 --- a/src/Runner.Worker/FileCommandManager.cs +++ b/src/Runner.Worker/FileCommandManager.cs @@ -262,11 +262,11 @@ private static string ReadLine( public sealed class SetStepSummaryCommand : RunnerService, IFileCommandExtension { + private const int _attachmentSizeLimit = 128 * 1024; + public string ContextName => "step_summary"; public string FilePrefix => "step_summary_"; - private const int AttachmentSizeLimit = 128 * 1024; - public Type ExtensionType => typeof(IFileCommandExtension); public void ProcessCommand(IExecutionContext context, string filePath, ContainerInfo container) @@ -307,9 +307,9 @@ private bool ShouldUploadAttachment(IExecutionContext context, string filePath) return false; } - if (fileSize > AttachmentSizeLimit) + if (fileSize > _attachmentSizeLimit) { - context.Error($"$GITHUB_STEP_SUMMARY supports content up a size of {AttachmentSizeLimit/1024}k got {fileSize/1024}k"); + context.Error($"$GITHUB_STEP_SUMMARY supports content up a size of {_attachmentSizeLimit/1024}k got {fileSize/1024}k"); Trace.Info($"Step Summary file ({filePath}) is too large ({fileSize} bytes); skipping attachment upload"); return false; @@ -340,8 +340,7 @@ private string ScrubStepSummaryFileSecrets(string filePath) private void QueueStepSummaryUpload(IExecutionContext context, string filePath) { - var stepID = context.Id; - var attachmentName = stepID.ToString(); + var attachmentName = context.Id.ToString(); Trace.Info($"Queueing file ({filePath}) for attachment upload ({attachmentName})"); try From 27decc4bc10418b4559abb47159f5e67b5cc1520 Mon Sep 17 00:00:00 2001 From: Sven Pfleiderer Date: Thu, 3 Feb 2022 19:06:33 +0000 Subject: [PATCH 23/29] inline check for feature flag --- src/Runner.Worker/ActionCommandManager.cs | 5 ----- src/Runner.Worker/FileCommandManager.cs | 2 +- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/src/Runner.Worker/ActionCommandManager.cs b/src/Runner.Worker/ActionCommandManager.cs index 1c48109aa32..6375db2934a 100644 --- a/src/Runner.Worker/ActionCommandManager.cs +++ b/src/Runner.Worker/ActionCommandManager.cs @@ -191,11 +191,6 @@ internal static bool EnhancedAnnotationsEnabled(IExecutionContext context) { return context.Global.Variables.GetBoolean("DistributedTask.EnhancedAnnotations") ?? false; } - - internal static bool StepSummaryEnabled(IExecutionContext context) - { - return context.Global.Variables.GetBoolean("DistributedTask.UploadStepSummary") ?? false; - } } public interface IActionCommandExtension : IExtension diff --git a/src/Runner.Worker/FileCommandManager.cs b/src/Runner.Worker/FileCommandManager.cs index e087cc203da..20e646a488d 100644 --- a/src/Runner.Worker/FileCommandManager.cs +++ b/src/Runner.Worker/FileCommandManager.cs @@ -282,7 +282,7 @@ public void ProcessCommand(IExecutionContext context, string filePath, Container private bool ShouldUploadAttachment(IExecutionContext context, string filePath) { - if (!ActionCommandManager.StepSummaryEnabled(context)) + if (!context.Global.Variables.GetBoolean("DistributedTask.UploadStepSummary") ?? false) { Trace.Info("Step Summary is disabled; skipping attachment upload"); return false; From 2c455ff13ce97a6e31787e1f7f4fde857755f570 Mon Sep 17 00:00:00 2001 From: Sven Pfleiderer Date: Thu, 3 Feb 2022 19:31:33 +0000 Subject: [PATCH 24/29] Inline method for style consistency --- src/Runner.Worker/FileCommandManager.cs | 75 +++++++++---------------- 1 file changed, 25 insertions(+), 50 deletions(-) diff --git a/src/Runner.Worker/FileCommandManager.cs b/src/Runner.Worker/FileCommandManager.cs index 20e646a488d..fd889ef2721 100644 --- a/src/Runner.Worker/FileCommandManager.cs +++ b/src/Runner.Worker/FileCommandManager.cs @@ -271,85 +271,60 @@ public sealed class SetStepSummaryCommand : RunnerService, IFileCommandExtension public void ProcessCommand(IExecutionContext context, string filePath, ContainerInfo container) { - if (ShouldUploadAttachment(context, filePath)) - { - Trace.Info($"Submitting step summary content from file {filePath}, container: {container}"); - var scrubbedFilePath = ScrubStepSummaryFileSecrets(filePath); - - QueueStepSummaryUpload(context, scrubbedFilePath); - } - } - - private bool ShouldUploadAttachment(IExecutionContext context, string filePath) - { - if (!context.Global.Variables.GetBoolean("DistributedTask.UploadStepSummary") ?? false) + if (!context.Global.Variables.GetBoolean("DistributedTask.UploadStepSummary") ?? true) { Trace.Info("Step Summary is disabled; skipping attachment upload"); - return false; + return; } - if (String.IsNullOrEmpty(filePath)) - { - Trace.Info("Step Summary path is empty; skipping attachment upload"); - return false; - } - - if (!File.Exists(filePath)) + if (String.IsNullOrEmpty(filePath) || !File.Exists(filePath)) { Trace.Info($"Step Summary file ({filePath}) does not exist; skipping attachment upload"); - return false; + return; } var fileSize = new FileInfo(filePath).Length; if (fileSize == 0) { Trace.Info($"Step Summary file ({filePath}) is empty; skipping attachment upload"); - return false; + return; } if (fileSize > _attachmentSizeLimit) { - context.Error($"$GITHUB_STEP_SUMMARY supports content up a size of {_attachmentSizeLimit/1024}k got {fileSize/1024}k"); + context.Error($"$GITHUB_STEP_SUMMARY supports content up a size of {_attachmentSizeLimit / 1024}k got {fileSize / 1024}k"); Trace.Info($"Step Summary file ({filePath}) is too large ({fileSize} bytes); skipping attachment upload"); - return false; + return; } - Trace.Info($"Step Summary file exists: {filePath} and has a file size of {fileSize} bytes"); + Trace.Verbose($"Step Summary file exists: {filePath} and has a file size of {fileSize} bytes"); - return true; - } - - private string ScrubStepSummaryFileSecrets(string filePath) - { - var scrubbedFilePath = filePath + "-scrubbed"; - - using (var streamReader = new StreamReader(filePath)) - using (var streamWriter = new StreamWriter(scrubbedFilePath)) + try { - string line; - while ((line = streamReader.ReadLine()) != null) + Trace.Info($"Submitting step summary content from file {filePath}, container: {container}"); + var scrubbedFilePath = filePath + "-scrubbed"; + + using (var streamReader = new StreamReader(filePath)) + using (var streamWriter = new StreamWriter(scrubbedFilePath)) { - var maskedLine = HostContext.SecretMasker.MaskSecrets(line); - streamWriter.WriteLine(maskedLine); + string line; + while ((line = streamReader.ReadLine()) != null) + { + var maskedLine = HostContext.SecretMasker.MaskSecrets(line); + streamWriter.WriteLine(maskedLine); + } } - } - return scrubbedFilePath; - } + var attachmentName = context.Id.ToString(); - private void QueueStepSummaryUpload(IExecutionContext context, string filePath) - { - var attachmentName = context.Id.ToString(); - - Trace.Info($"Queueing file ({filePath}) for attachment upload ({attachmentName})"); - try - { - context.QueueAttachFile(ChecksAttachmentType.StepSummary, attachmentName, filePath); + Trace.Info($"Queueing file ({filePath}) for attachment upload ({attachmentName})"); + context.QueueAttachFile(ChecksAttachmentType.StepSummary, attachmentName, scrubbedFilePath); } catch (Exception e) { - Trace.Error($"Error while trying to enqueue file upload for file ({filePath}): {e}"); + Trace.Error($"Error while processing file file ({filePath}): {e}"); + context.Error($"Error while enqueueing GITHUB_STEP_SUMMARY: {e.Message}"); } } } From bfdfa442a4e5187bce9d7cb754f85b9f4e9c323c Mon Sep 17 00:00:00 2001 From: Sven Pfleiderer Date: Thu, 3 Feb 2022 22:30:35 +0000 Subject: [PATCH 25/29] Make sure that scrubbed file doesn't exist before creating it --- src/Runner.Worker/FileCommandManager.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/Runner.Worker/FileCommandManager.cs b/src/Runner.Worker/FileCommandManager.cs index fd889ef2721..8c1a50ebe58 100644 --- a/src/Runner.Worker/FileCommandManager.cs +++ b/src/Runner.Worker/FileCommandManager.cs @@ -305,6 +305,11 @@ public void ProcessCommand(IExecutionContext context, string filePath, Container Trace.Info($"Submitting step summary content from file {filePath}, container: {container}"); var scrubbedFilePath = filePath + "-scrubbed"; + if (File.Exists(scrubbedFilePath)) + { + File.Delete(scrubbedFilePath); + } + using (var streamReader = new StreamReader(filePath)) using (var streamWriter = new StreamWriter(scrubbedFilePath)) { From 29680f5c68505c967599a2b0d27ab842b6a2690f Mon Sep 17 00:00:00 2001 From: Sven Pfleiderer Date: Thu, 3 Feb 2022 22:34:58 +0000 Subject: [PATCH 26/29] Rename SetStepSummaryCommand to CreateStepSummaryCommand --- src/Runner.Worker/FileCommandManager.cs | 32 +++++++------- ...andL0.cs => CreateStepSummaryCommandL0.cs} | 42 +++++++++---------- 2 files changed, 36 insertions(+), 38 deletions(-) rename src/Test/L0/Worker/{SetStepSummaryCommandL0.cs => CreateStepSummaryCommandL0.cs} (84%) diff --git a/src/Runner.Worker/FileCommandManager.cs b/src/Runner.Worker/FileCommandManager.cs index 8c1a50ebe58..24563ba1063 100644 --- a/src/Runner.Worker/FileCommandManager.cs +++ b/src/Runner.Worker/FileCommandManager.cs @@ -260,7 +260,7 @@ private static string ReadLine( } } - public sealed class SetStepSummaryCommand : RunnerService, IFileCommandExtension + public sealed class CreateStepSummaryCommand : RunnerService, IFileCommandExtension { private const int _attachmentSizeLimit = 128 * 1024; @@ -283,26 +283,24 @@ public void ProcessCommand(IExecutionContext context, string filePath, Container return; } - var fileSize = new FileInfo(filePath).Length; - if (fileSize == 0) - { - Trace.Info($"Step Summary file ({filePath}) is empty; skipping attachment upload"); - return; - } - - if (fileSize > _attachmentSizeLimit) + try { - context.Error($"$GITHUB_STEP_SUMMARY supports content up a size of {_attachmentSizeLimit / 1024}k got {fileSize / 1024}k"); - Trace.Info($"Step Summary file ({filePath}) is too large ({fileSize} bytes); skipping attachment upload"); + var fileSize = new FileInfo(filePath).Length; + if (fileSize == 0) + { + Trace.Info($"Step Summary file ({filePath}) is empty; skipping attachment upload"); + return; + } - return; - } + if (fileSize > _attachmentSizeLimit) + { + context.Error($"$GITHUB_STEP_SUMMARY supports content up a size of {_attachmentSizeLimit / 1024}k got {fileSize / 1024}k"); + Trace.Info($"Step Summary file ({filePath}) is too large ({fileSize} bytes); skipping attachment upload"); - Trace.Verbose($"Step Summary file exists: {filePath} and has a file size of {fileSize} bytes"); + return; + } - try - { - Trace.Info($"Submitting step summary content from file {filePath}, container: {container}"); + Trace.Verbose($"Step Summary file exists: {filePath} and has a file size of {fileSize} bytes"); var scrubbedFilePath = filePath + "-scrubbed"; if (File.Exists(scrubbedFilePath)) diff --git a/src/Test/L0/Worker/SetStepSummaryCommandL0.cs b/src/Test/L0/Worker/CreateStepSummaryCommandL0.cs similarity index 84% rename from src/Test/L0/Worker/SetStepSummaryCommandL0.cs rename to src/Test/L0/Worker/CreateStepSummaryCommandL0.cs index 8cb1c84a90f..f75fb8e0170 100644 --- a/src/Test/L0/Worker/SetStepSummaryCommandL0.cs +++ b/src/Test/L0/Worker/CreateStepSummaryCommandL0.cs @@ -13,19 +13,19 @@ namespace GitHub.Runner.Common.Tests.Worker { - public sealed class SetStepSummaryCommandL0 + public sealed class CreateStepSummaryCommandL0 { private Mock _executionContext; private List> _issues; private Variables _variables; private string _rootDirectory; - private SetStepSummaryCommand _setStepCommand; + private CreateStepSummaryCommand _createStepCommand; private ITraceWriter _trace; [Fact] [Trait("Level", "L0")] [Trait("Category", "Worker")] - public void SetStepSummaryCommand_FeatureDisabled() + public void CreateStepSummaryCommand_FeatureDisabled() { using (var hostContext = Setup(featureFlagState: "false")) { @@ -40,11 +40,11 @@ public void SetStepSummaryCommand_FeatureDisabled() [Fact] [Trait("Level", "L0")] [Trait("Category", "Worker")] - public void SetStepSummaryCommand_FileNull() + public void CreateStepSummaryCommand_FileNull() { using (var hostContext = Setup()) { - _setStepCommand.ProcessCommand(_executionContext.Object, null, null); + _createStepCommand.ProcessCommand(_executionContext.Object, null, null); _executionContext.Verify(e => e.QueueAttachFile(ChecksAttachmentType.StepSummary, It.IsAny(), It.IsAny()), Times.Never()); Assert.Equal(0, _issues.Count); @@ -54,13 +54,13 @@ public void SetStepSummaryCommand_FileNull() [Fact] [Trait("Level", "L0")] [Trait("Category", "Worker")] - public void SetStepSummaryCommand_DirectoryNotFound() + public void CreateStepSummaryCommand_DirectoryNotFound() { using (var hostContext = Setup()) { var stepSummaryFile = Path.Combine(_rootDirectory, "directory-not-found", "env"); - _setStepCommand.ProcessCommand(_executionContext.Object, stepSummaryFile, null); + _createStepCommand.ProcessCommand(_executionContext.Object, stepSummaryFile, null); _executionContext.Verify(e => e.QueueAttachFile(ChecksAttachmentType.StepSummary, It.IsAny(), It.IsAny()), Times.Never()); Assert.Equal(0, _issues.Count); @@ -70,13 +70,13 @@ public void SetStepSummaryCommand_DirectoryNotFound() [Fact] [Trait("Level", "L0")] [Trait("Category", "Worker")] - public void SetStepSummaryCommand_FileNotFound() + public void CreateStepSummaryCommand_FileNotFound() { using (var hostContext = Setup()) { var stepSummaryFile = Path.Combine(_rootDirectory, "file-not-found"); - _setStepCommand.ProcessCommand(_executionContext.Object, stepSummaryFile, null); + _createStepCommand.ProcessCommand(_executionContext.Object, stepSummaryFile, null); _executionContext.Verify(e => e.QueueAttachFile(ChecksAttachmentType.StepSummary, It.IsAny(), It.IsAny()), Times.Never()); Assert.Equal(0, _issues.Count); @@ -86,14 +86,14 @@ public void SetStepSummaryCommand_FileNotFound() [Fact] [Trait("Level", "L0")] [Trait("Category", "Worker")] - public void SetStepSummaryCommand_EmptyFile() + public void CreateStepSummaryCommand_EmptyFile() { using (var hostContext = Setup()) { var stepSummaryFile = Path.Combine(_rootDirectory, "empty-file"); File.Create(stepSummaryFile).Dispose(); - _setStepCommand.ProcessCommand(_executionContext.Object, stepSummaryFile, null); + _createStepCommand.ProcessCommand(_executionContext.Object, stepSummaryFile, null); _executionContext.Verify(e => e.QueueAttachFile(ChecksAttachmentType.StepSummary, It.IsAny(), It.IsAny()), Times.Never()); Assert.Equal(0, _issues.Count); @@ -103,14 +103,14 @@ public void SetStepSummaryCommand_EmptyFile() [Fact] [Trait("Level", "L0")] [Trait("Category", "Worker")] - public void SetStepSummaryCommand_LargeFile() + public void CreateStepSummaryCommand_LargeFile() { using (var hostContext = Setup()) { var stepSummaryFile = Path.Combine(_rootDirectory, "empty-file"); File.WriteAllBytes(stepSummaryFile, new byte[128 * 1024 + 1]); - _setStepCommand.ProcessCommand(_executionContext.Object, stepSummaryFile, null); + _createStepCommand.ProcessCommand(_executionContext.Object, stepSummaryFile, null); _executionContext.Verify(e => e.QueueAttachFile(ChecksAttachmentType.StepSummary, It.IsAny(), It.IsAny()), Times.Never()); Assert.Equal(1, _issues.Count); @@ -120,7 +120,7 @@ public void SetStepSummaryCommand_LargeFile() [Fact] [Trait("Level", "L0")] [Trait("Category", "Worker")] - public void SetStepSummaryCommand_Simple() + public void CreateStepSummaryCommand_Simple() { using (var hostContext = Setup()) { @@ -133,7 +133,7 @@ public void SetStepSummaryCommand_Simple() }; WriteContent(stepSummaryFile, content); - _setStepCommand.ProcessCommand(_executionContext.Object, stepSummaryFile, null); + _createStepCommand.ProcessCommand(_executionContext.Object, stepSummaryFile, null); _executionContext.Verify(e => e.QueueAttachFile(ChecksAttachmentType.StepSummary, _executionContext.Object.Id.ToString(), stepSummaryFile + "-scrubbed"), Times.Once()); Assert.Equal(0, _issues.Count); @@ -143,7 +143,7 @@ public void SetStepSummaryCommand_Simple() [Fact] [Trait("Level", "L0")] [Trait("Category", "Worker")] - public void SetStepSummaryCommand_ScrubSecrets() + public void CreateStepSummaryCommand_ScrubSecrets() { using (var hostContext = Setup()) { @@ -161,7 +161,7 @@ public void SetStepSummaryCommand_ScrubSecrets() }; WriteContent(stepSummaryFile, content); - _setStepCommand.ProcessCommand(_executionContext.Object, stepSummaryFile, null); + _createStepCommand.ProcessCommand(_executionContext.Object, stepSummaryFile, null); var scrubbedFileContents = File.ReadAllText(scrubbedFile); Assert.DoesNotContain("ThisIsMySecretPassword!", scrubbedFileContents); @@ -206,7 +206,7 @@ private TestHostContext Setup([CallerMemberName] string name = "", string featur var workDirectory = hostContext.GetDirectory(WellKnownDirectory.Work); ArgUtil.NotNullOrEmpty(workDirectory, nameof(workDirectory)); Directory.CreateDirectory(workDirectory); - _rootDirectory = Path.Combine(workDirectory, nameof(SetStepSummaryCommandL0)); + _rootDirectory = Path.Combine(workDirectory, nameof(CreateStepSummaryCommandL0)); Directory.CreateDirectory(_rootDirectory); // Execution context @@ -231,9 +231,9 @@ private TestHostContext Setup([CallerMemberName] string name = "", string featur _trace.Info($"{tag}{message}"); }); - // SetStepSummaryCommand - _setStepCommand = new SetStepSummaryCommand(); - _setStepCommand.Initialize(hostContext); + //CreateStepSummaryCommand + _createStepCommand = new CreateStepSummaryCommand(); + _createStepCommand.Initialize(hostContext); return hostContext; } From 03182cf8614e959123a3c29dc3719f0afb944254 Mon Sep 17 00:00:00 2001 From: Sven Pfleiderer Date: Thu, 3 Feb 2022 22:43:50 +0000 Subject: [PATCH 27/29] Fix error handling messages --- src/Runner.Worker/FileCommandManager.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Runner.Worker/FileCommandManager.cs b/src/Runner.Worker/FileCommandManager.cs index 24563ba1063..8947ee7e482 100644 --- a/src/Runner.Worker/FileCommandManager.cs +++ b/src/Runner.Worker/FileCommandManager.cs @@ -326,8 +326,8 @@ public void ProcessCommand(IExecutionContext context, string filePath, Container } catch (Exception e) { - Trace.Error($"Error while processing file file ({filePath}): {e}"); - context.Error($"Error while enqueueing GITHUB_STEP_SUMMARY: {e.Message}"); + Trace.Error($"Error while processing file ({filePath}): {e}"); + context.Error($"Failed to create step summary using 'GITHUB_STEP_SUMMARY': {e.Message}"); } } } From e46424a77451d383b7327127549b704335f147ec Mon Sep 17 00:00:00 2001 From: Sven Pfleiderer Date: Fri, 4 Feb 2022 21:07:33 +0000 Subject: [PATCH 28/29] Fix file command name when registering extension --- src/Runner.Common/ExtensionManager.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Runner.Common/ExtensionManager.cs b/src/Runner.Common/ExtensionManager.cs index daeebf7126c..29f99b62834 100644 --- a/src/Runner.Common/ExtensionManager.cs +++ b/src/Runner.Common/ExtensionManager.cs @@ -60,7 +60,7 @@ private List LoadExtensions() where T : class, IExtension case "GitHub.Runner.Worker.IFileCommandExtension": Add(extensions, "GitHub.Runner.Worker.AddPathFileCommand, Runner.Worker"); Add(extensions, "GitHub.Runner.Worker.SetEnvFileCommand, Runner.Worker"); - Add(extensions, "GitHub.Runner.Worker.SetStepSummaryCommand, Runner.Worker"); + Add(extensions, "GitHub.Runner.Worker.CreateStepSummaryCommand, Runner.Worker"); break; case "GitHub.Runner.Listener.Check.ICheckExtension": Add(extensions, "GitHub.Runner.Listener.Check.InternetCheck, Runner.Listener"); From 8ce4203ec42b328f0355fbd7d735bda7f0d58e8b Mon Sep 17 00:00:00 2001 From: Sven Pfleiderer Date: Fri, 4 Feb 2022 21:14:56 +0000 Subject: [PATCH 29/29] Remove unnecessary file deletion --- src/Runner.Worker/FileCommandManager.cs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/Runner.Worker/FileCommandManager.cs b/src/Runner.Worker/FileCommandManager.cs index 8947ee7e482..f45528a5c17 100644 --- a/src/Runner.Worker/FileCommandManager.cs +++ b/src/Runner.Worker/FileCommandManager.cs @@ -303,11 +303,6 @@ public void ProcessCommand(IExecutionContext context, string filePath, Container Trace.Verbose($"Step Summary file exists: {filePath} and has a file size of {fileSize} bytes"); var scrubbedFilePath = filePath + "-scrubbed"; - if (File.Exists(scrubbedFilePath)) - { - File.Delete(scrubbedFilePath); - } - using (var streamReader = new StreamReader(filePath)) using (var streamWriter = new StreamWriter(scrubbedFilePath)) {