Improve Error Messaging for Actions by Using ExecutionContext's FileTable as Single Source of Truth and by Passing FileID to All Children Tokens. #564

ethanchewy · 2020-06-23T17:05:32Z

Branched off from: #557

This PR enables file names to be included in action error messages.

Our goal is to ensure that the fileIDs are associated with all the children tokens in the action.yml so that error messages reference the correct action file to allow for easier debugging. Also, we want to make sure that the FileTable is updated correctly throughout the process to ensure that Error messages will reference always the correctly associated file.

In this PR, we also refactor the code in ActionManifestManager to allow for passing fileIDs to the children tokens. Originally, there was a _fileTable attribute for ActionManifestManager instances as well as a FileTable for ExecutionContext instances. To reduce confusion and potential bugs in the future when we have nested actions, we refactored how the FileTable is used so that only ExecutionContext.FileTable is referenced. In the ActionManifestManager to ensure that only 1 fileTable (aka the ExecutionContext's fileTable) is referenced. Moreover, we add functionality to ensure that the FileTable is updated every time we load an Action yaml.

tl;dr ExecutionContext's FileTable is now the "source of truth" for any filetable related matters.

Demos:

=> Before error message: https://github.com/ethanchewy/testing-actions/actions/runs/144061679
(Line: 13, Col: 11): Unexpected value 'run:::::'
=> After PR error message: https://github.com/ethanchewy/testing-actions/actions/runs/145235133
ethanchewy/testing-composite-errors/v3/action.yml (Line: 13, Col: 11): Unexpected value 'run:::::'

More demos:
action.yml

runs:
    using: "composite"
    steps: 
        - run: echo ${{ fromJson("not json") }}

Error Messaging after delayed evaluation:
https://github.com/ethanchewy/testing-actions/actions/runs/145233958
ethanchewy/testing-composite-errors/v4/action.yml (Line: 13, Col: 16): Unexpected symbol: '"not'. Located at position 10 within expression: fromJson("not json"))

…bles => but it doesn't 'stick'

…actions/runner into users/ethanchewy/compositeenv

ethanchewy · 2020-06-23T17:08:09Z

~~TODO: git pull master after merging the Composite Run Steps PR or git pull origin users/ethanchewy/compositetest2~~
Done.

ethanchewy · 2020-06-23T17:10:53Z

~~TODO: Test with error messages such as {{ fromJson('not json') }}~~
Done.

TingluoHuang · 2020-07-08T14:48:57Z

src/Runner.Worker/ActionManifestManager.cs

+            // 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);


HostContext.GetDirectory()

TingluoHuang · 2020-07-08T14:51:08Z

src/Runner.Worker/ActionManifestManager.cs

                }

-                throw new ArgumentException($"Fail to load {manifestFile}");
+                throw new ArgumentException($"Fail to load {fileName}");


will this always shows actions.yml?

TODO for ethan: Change variable name top a more clear name

discussed offline, it's the relative path. Would it help to name the variable relative___?

TingluoHuang · 2020-07-08T14:53:39Z

src/Sdk/DTObjectTemplating/ObjectTemplating/TemplateEvaluator.cs

            Boolean omitHeader = false)
        {
            TemplateToken result;
-


undo this. 😆

TingluoHuang · 2020-07-08T14:53:53Z

src/Sdk/DTPipelines/Pipelines/ObjectTemplating/PipelineTemplateEvaluator.cs

        }

        public List<ActionStep> LoadCompositeSteps(
-            TemplateToken token)


need to copy this change back to server.

ericsciple · 2020-07-08T14:56:45Z

src/Runner.Worker/ActionManifestManager.cs

+                var fileId = templateContext.GetFileId(fileName);
+
+                // Add this file to the FileTable in executionContext
+                executionContext.FileTable.Add(fileName);


After you add the "fileName" the fileId should match executionContext.FileTable.Length

only add if new

ericsciple · 2020-07-08T15:00:29Z

src/Runner.Worker/ActionManifestManager.cs

            TemplateContext context,
-            TemplateToken inputsToken)
+            TemplateToken inputsToken,
+            Int32 fileID)


is this used?

ericsciple · 2020-07-08T15:01:15Z

src/Runner.Worker/Handlers/ScriptHandler.cs

                if (exitCode != 0)
                {
-                    ExecutionContext.Error($"Process completed with exit code {exitCode}.");
+                    ExecutionContext.Error($"Process completed with exit code {exitCode}");


ericsciple · 2020-07-08T15:02:49Z

src/Sdk/DTPipelines/Pipelines/ObjectTemplating/PipelineTemplateEvaluator.cs


        public List<ActionStep> LoadCompositeSteps(
-            TemplateToken token)
+            TemplateToken token,


dont forget to revert this

…/ethanchewy/compositeFileTable

…file to fileTable

ericsciple · 2020-07-08T20:13:37Z

src/Runner.Worker/ActionManifestManager.cs

+
+                // 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)


do we need FileTable? anymore, or should this just be FileTable now?

Yeah, we don't need it anymore since the FileTable should always be non null. Will update this to executionContext.FileTable.Count.

good catch -ty

ericsciple · 2020-07-08T20:14:36Z

src/Runner.Worker/ActionManifestManager.cs

-            // Add the file table
-            if (_fileTable?.Count > 0)
+            // Add the file table from the Execution Context
+            if (executionContext.FileTable?.Count > 0)


same question here, i think we can drop the ? now

…d never be non null

…able as Single Source of Truth and by Passing FileID to All Children Tokens. (actions#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

ethanchewy added 27 commits June 18, 2020 09:48

Composite Action Run Steps

038e5e2

Env Flow => Able to get env variables and overwrite current env varia…

e56b243

…bles => but it doesn't 'stick'

clean up

6552263

Clean up trace messages + add Trace debug in ActionManager

180a687

Merge branch 'users/ethanchewy/compositetest2' of https://github.com/…

66cadeb

…actions/runner into users/ethanchewy/compositeenv

Add debugging message

96e0037

Optimize runtime of code

496064f

Change String to string

5843362

Add comma to Composite

941a24e

Merge branch 'users/ethanchewy/compositetest2' of https://github.com/…

28be3df

…actions/runner into users/ethanchewy/compositeenv

Change JobSteps to a List, Change Register Step function name

9939cf5

Add TODO, remove unn. content

5988076

Remove unnecessary code

00a736d

Fix unit tests

e4dfd0e

Add/Merge changes from Multiple Steps PR

f8054f9

Fix env format

45ddd42

Remove comment

37849dc

Remove TODO message for context

94e7b47

Add verbose trace logs which are only viewable by devs

a254442

Merge branch 'users/ethanchewy/compositetest2' of https://github.com/…

fbef557

…actions/runner into users/ethanchewy/compositeenv

Initial Start for FileTable stuff

43a3006

Progress towards passing FileTable or FileID or FileName

58d11ef

Sort usings in Composite Action Handler

0d5e84b

Change 0 to location

9ec7047

Update context variables in composite action yaml

8aadbbd

Add helpful error message for null steps

f9b28c7

Pass fileID to all children token of root action token

da2da85

ethanchewy changed the title ~~(IN PROGRESS) Pass FileId to Children Tokens and Only Use Execution Context's FileTable in ActionManifestManager~~ (IN PROGRESS) Add Action File Info in ActionManifestManager and Pass Related FileID to Children Nodes for Better Error Messaging Jun 23, 2020

TingluoHuang reviewed Jul 8, 2020

View reviewed changes

Fix period

3836574

TingluoHuang reviewed Jul 8, 2020

View reviewed changes

ericsciple reviewed Jul 8, 2020

View reviewed changes

ericsciple approved these changes Jul 8, 2020

View reviewed changes

ethanchewy added 3 commits July 8, 2020 11:14

Sanity check for fileTable add + remove unn. code

9a1dd80

revert back

d2d0ecf

Add back line break

41a8c8c

ethanchewy changed the base branch from users/ethanchewy/compositeenv to master July 8, 2020 15:21

ethanchewy changed the base branch from master to users/ethanchewy/compositeenv July 8, 2020 15:21

ethanchewy changed the base branch from users/ethanchewy/compositeenv to master July 8, 2020 15:31

ethanchewy added 8 commits July 8, 2020 11:33

Merge branch 'master' of https://github.com/actions/runner into users…

7c57d41

…/ethanchewy/compositeFileTable

Fix null errors

35879fc

Address situation if FileTable is null + add sanity check for adding …

8828263

…file to fileTable

add line

6d7efa9

Revert

9a2b3e2

Fix unit tests to instantiate a FileTable

b3eea21

Fix logic for trimming manifestfile path

af9202b

Add null check

635baa5

ericsciple reviewed Jul 8, 2020

View reviewed changes

Add filetable to testing file, remove ? since we know filetable shoul…

68b05f6

…d never be non null

ethanchewy merged commit 9d7bd47 into master Jul 8, 2020

ethanchewy mentioned this pull request Aug 7, 2020

Next Steps for Fully Functioning Composite Actions #646

Closed

Improve Error Messaging for Actions by Using ExecutionContext's FileTable as Single Source of Truth and by Passing FileID to All Children Tokens. #564

Improve Error Messaging for Actions by Using ExecutionContext's FileTable as Single Source of Truth and by Passing FileID to All Children Tokens. #564

Uh oh!

Conversation

ethanchewy commented Jun 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Demos:

Uh oh!

ethanchewy commented Jun 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ethanchewy commented Jun 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericsciple Jul 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericsciple Jul 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ethanchewy commented Jun 23, 2020 •

edited

Loading

ethanchewy commented Jun 23, 2020 •

edited

Loading

ethanchewy commented Jun 23, 2020 •

edited

Loading

ericsciple Jul 8, 2020 •

edited

Loading

ericsciple Jul 8, 2020 •

edited

Loading