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

@@ -33,8 +33,6 @@ namespace GitHub.Runner.Worker
     public sealed class ActionManifestManager : RunnerService, IActionManifestManager
     {
         private TemplateSchema _actionManifestSchema;
-        private IReadOnlyList<String> _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);
+                var fileId = templateContext.GetFileId(fileRelativePath);
-                _fileTable = context.GetFileTable();
+
+                // 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);
+                    var yamlObjectReader = new YamlObjectReader(fileId, stringReader);
-                    token = TemplateReader.Read(context, "action-root", yamlObjectReader, fileId, out _);
+                    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
+            // Add the file table from the Execution Context
-            if (_fileTable?.Count > 0)
+            for (var i = 0; i < executionContext.FileTable.Count; i++)
             {
-                for (var i = 0; i < _fileTable.Count; i++)
+                result.GetFileId(executionContext.FileTable[i]);
-                {
-                    result.GetFileId(_fileTable[i]);
-                }
             }
 
             return result;

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<IFunctionInfo>());
+            _ec.Setup(x => x.FileTable).Returns(new List<String>());
             _ec.Setup(x => x.Plan).Returns(new TaskOrchestrationPlanReference());
             _ec.Setup(x => x.Write(It.IsAny<string>(), It.IsAny<string>())).Callback((string tag, string message) => { _hc.GetTrace().Info($"[{tag}]{message}"); });
             _ec.Setup(x => x.AddIssue(It.IsAny<Issue>(), It.IsAny<string>())).Callback((Issue issue, string message) => { _hc.GetTrace().Info($"[{issue.Type}]{issue.Message ?? message}"); });

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<string, VariableValue>()));
             _ec.Setup(x => x.ExpressionValues).Returns(new DictionaryContextData());
             _ec.Setup(x => x.ExpressionFunctions).Returns(new List<IFunctionInfo>());
+            _ec.Setup(x => x.FileTable).Returns(new List<String>());
             _ec.Setup(x => x.Write(It.IsAny<string>(), It.IsAny<string>())).Callback((string tag, string message) => { _hc.GetTrace().Info($"{tag}{message}"); });
             _ec.Setup(x => x.AddIssue(It.IsAny<Issue>(), It.IsAny<string>())).Callback((Issue issue, string message) => { _hc.GetTrace().Info($"[{issue.Type}]{issue.Message ?? message}"); });
         }

src/Test/L0/Worker/ActionRunnerL0.cs

@@ -379,6 +379,7 @@ namespace GitHub.Runner.Common.Tests.Worker
             _ec.Setup(x => x.ExpressionFunctions).Returns(new List<IFunctionInfo>());
             _ec.Setup(x => x.IntraActionState).Returns(new Dictionary<string, string>());
             _ec.Setup(x => x.EnvironmentVariables).Returns(new Dictionary<string, string>());
+            _ec.Setup(x => x.FileTable).Returns(new List<String>());
             _ec.Setup(x => x.SetGitHubContext(It.IsAny<string>(), It.IsAny<string>()));
             _ec.Setup(x => x.GetGitHubContext(It.IsAny<string>())).Returns("{\"foo\":\"bar\"}");
             _ec.Setup(x => x.CancellationToken).Returns(_ecTokenSource.Token);