From 9d7bd4706b5e96108f09f2b9fd32c5d170d5cec2 Mon Sep 17 00:00:00 2001 From: Ethan Chiu <17chiue@gmail.com> Date: Wed, 8 Jul 2020 17:15:16 -0400 Subject: [PATCH] Improve Error Messaging for Actions by Using ExecutionContext's FileTable as Single Source of Truth and by Passing FileID to All Children Tokens. (#564) * Composite Action Run Steps * Env Flow => Able to get env variables and overwrite current env variables => but it doesn't 'stick' * clean up * Clean up trace messages + add Trace debug in ActionManager * Add debugging message * Optimize runtime of code * Change String to string * Add comma to Composite * Change JobSteps to a List, Change Register Step function name * Add TODO, remove unn. content * Remove unnecessary code * Fix unit tests * Fix env format * Remove comment * Remove TODO message for context * Add verbose trace logs which are only viewable by devs * Initial Start for FileTable stuff * Progress towards passing FileTable or FileID or FileName * Sort usings in Composite Action Handler * Change 0 to location * Update context variables in composite action yaml * Add helpful error message for null steps * Pass fileID to all children token of root action token * Change confusing term context => templateContext, Eliminate _fileTable and only use ExecutionContext.FileTable + update this table when need be * Remove unnessary FileID attribute from CompositeActionExecutionData * Clean up file path for error message * Remove todo * Fix Workflow Step Env overiding Parent Env * Remove env in composite action scope * Clean up * Revert back * revert back * add back envToken * Remove unnecessary code * Add file length check * Clean up * Figure out how to handle set-env edge cases * formatting * fix unit tests * Fix windows unit test syntax error * Fix period * Sanity check for fileTable add + remove unn. code * revert back * Add back line break * Fix null errors * Address situation if FileTable is null + add sanity check for adding file to fileTable * add line * Revert * Fix unit tests to instantiate a FileTable * Fix logic for trimming manifestfile path * Add null check * Add filetable to testing file, remove ? since we know filetable should never be non null --- src/Runner.Worker/ActionManifestManager.cs | 52 ++++++++++++------- src/Test/L0/Worker/ActionManagerL0.cs | 1 + src/Test/L0/Worker/ActionManifestManagerL0.cs | 1 + src/Test/L0/Worker/ActionRunnerL0.cs | 1 + 4 files changed, 35 insertions(+), 20 deletions(-) diff --git a/src/Runner.Worker/ActionManifestManager.cs b/src/Runner.Worker/ActionManifestManager.cs index 9095f498d..ae5ca73d7 100644 --- a/src/Runner.Worker/ActionManifestManager.cs +++ b/src/Runner.Worker/ActionManifestManager.cs @@ -33,8 +33,6 @@ namespace GitHub.Runner.Worker public sealed class ActionManifestManager : RunnerService, IActionManifestManager { private TemplateSchema _actionManifestSchema; - private IReadOnlyList _fileTable; - public override void Initialize(IHostContext hostContext) { base.Initialize(hostContext); @@ -55,22 +53,39 @@ namespace GitHub.Runner.Worker public ActionDefinitionData Load(IExecutionContext executionContext, string manifestFile) { - var context = CreateContext(executionContext); + var templateContext = CreateContext(executionContext); ActionDefinitionData actionDefinition = new ActionDefinitionData(); + + // Clean up file name real quick + // Instead of using Regex which can be computationally expensive, + // we can just remove the # of characters from the fileName according to the length of the basePath + string basePath = HostContext.GetDirectory(WellKnownDirectory.Actions); + string fileRelativePath = manifestFile; + if (manifestFile.Contains(basePath)) + { + fileRelativePath = manifestFile.Remove(0, basePath.Length + 1); + } + try { var token = default(TemplateToken); // Get the file ID - var fileId = context.GetFileId(manifestFile); - _fileTable = context.GetFileTable(); + var fileId = templateContext.GetFileId(fileRelativePath); + + // Add this file to the FileTable in executionContext if it hasn't been added already + // we use > since fileID is 1 indexed + if (fileId > executionContext.FileTable.Count) + { + executionContext.FileTable.Add(fileRelativePath); + } // Read the file var fileContent = File.ReadAllText(manifestFile); using (var stringReader = new StringReader(fileContent)) { - var yamlObjectReader = new YamlObjectReader(null, stringReader); - token = TemplateReader.Read(context, "action-root", yamlObjectReader, fileId, out _); + var yamlObjectReader = new YamlObjectReader(fileId, stringReader); + token = TemplateReader.Read(templateContext, "action-root", yamlObjectReader, fileId, out _); } var actionMapping = token.AssertMapping("action manifest root"); @@ -89,11 +104,11 @@ namespace GitHub.Runner.Worker break; case "inputs": - ConvertInputs(context, actionPair.Value, actionDefinition); + ConvertInputs(templateContext, actionPair.Value, actionDefinition); break; case "runs": - actionDefinition.Execution = ConvertRuns(executionContext, context, actionPair.Value); + actionDefinition.Execution = ConvertRuns(executionContext, templateContext, actionPair.Value); break; default: Trace.Info($"Ignore action property {propertyName}."); @@ -104,24 +119,24 @@ namespace GitHub.Runner.Worker catch (Exception ex) { Trace.Error(ex); - context.Errors.Add(ex); + templateContext.Errors.Add(ex); } - if (context.Errors.Count > 0) + if (templateContext.Errors.Count > 0) { - foreach (var error in context.Errors) + foreach (var error in templateContext.Errors) { Trace.Error($"Action.yml load error: {error.Message}"); executionContext.Error(error.Message); } - throw new ArgumentException($"Fail to load {manifestFile}"); + throw new ArgumentException($"Fail to load {fileRelativePath}"); } if (actionDefinition.Execution == null) { executionContext.Debug($"Loaded action.yml file: {StringUtil.ConvertToJson(actionDefinition)}"); - throw new ArgumentException($"Top level 'runs:' section is required for {manifestFile}"); + throw new ArgumentException($"Top level 'runs:' section is required for {fileRelativePath}"); } else { @@ -282,13 +297,10 @@ namespace GitHub.Runner.Worker result.ExpressionFunctions.Add(item); } - // Add the file table - if (_fileTable?.Count > 0) + // Add the file table from the Execution Context + for (var i = 0; i < executionContext.FileTable.Count; i++) { - for (var i = 0; i < _fileTable.Count; i++) - { - result.GetFileId(_fileTable[i]); - } + result.GetFileId(executionContext.FileTable[i]); } return result; diff --git a/src/Test/L0/Worker/ActionManagerL0.cs b/src/Test/L0/Worker/ActionManagerL0.cs index 58ffea5aa..b1ccb284b 100644 --- a/src/Test/L0/Worker/ActionManagerL0.cs +++ b/src/Test/L0/Worker/ActionManagerL0.cs @@ -3584,6 +3584,7 @@ runs: _ec.Setup(x => x.Variables).Returns(new Variables(_hc, variables)); _ec.Setup(x => x.ExpressionValues).Returns(new DictionaryContextData()); _ec.Setup(x => x.ExpressionFunctions).Returns(new List()); + _ec.Setup(x => x.FileTable).Returns(new List()); _ec.Setup(x => x.Plan).Returns(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}"); }); diff --git a/src/Test/L0/Worker/ActionManifestManagerL0.cs b/src/Test/L0/Worker/ActionManifestManagerL0.cs index 07f99a0ae..737341922 100644 --- a/src/Test/L0/Worker/ActionManifestManagerL0.cs +++ b/src/Test/L0/Worker/ActionManifestManagerL0.cs @@ -759,6 +759,7 @@ namespace GitHub.Runner.Common.Tests.Worker _ec.Setup(x => x.Variables).Returns(new Variables(_hc, new Dictionary())); _ec.Setup(x => x.ExpressionValues).Returns(new DictionaryContextData()); _ec.Setup(x => x.ExpressionFunctions).Returns(new List()); + _ec.Setup(x => x.FileTable).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}"); }); } diff --git a/src/Test/L0/Worker/ActionRunnerL0.cs b/src/Test/L0/Worker/ActionRunnerL0.cs index 662ea307d..1851f47d7 100644 --- a/src/Test/L0/Worker/ActionRunnerL0.cs +++ b/src/Test/L0/Worker/ActionRunnerL0.cs @@ -379,6 +379,7 @@ namespace GitHub.Runner.Common.Tests.Worker _ec.Setup(x => x.ExpressionFunctions).Returns(new List()); _ec.Setup(x => x.IntraActionState).Returns(new Dictionary()); _ec.Setup(x => x.EnvironmentVariables).Returns(new Dictionary()); + _ec.Setup(x => x.FileTable).Returns(new List()); _ec.Setup(x => x.SetGitHubContext(It.IsAny(), It.IsAny())); _ec.Setup(x => x.GetGitHubContext(It.IsAny())).Returns("{\"foo\":\"bar\"}"); _ec.Setup(x => x.CancellationToken).Returns(_ecTokenSource.Token);