Composite Run Steps Outputs #568
Conversation
…bles => but it doesn't 'stick'
…e and only use ExecutionContext.FileTable + update this table when need be
…actions/runner into users/ethanchewy/compositeFileTable
|
|
||
| case "runs": | ||
| actionDefinition.Execution = ConvertRuns(executionContext, templateContext, actionPair.Value); | ||
| actionDefinition.Execution = ConvertRuns(executionContext, templateContext, actionPair.Value, actionOutputs); |
There was a problem hiding this comment.
Let's store runs (i.e. actionPair.Value) and then call ConvertRuns outside of the for loop. Otherwise this adds a constraint that the order matters. Order shouldn't matter for json (for yaml 🤷 but lets make it not matter).
|
|
||
| bool EchoOnActionCommand { get; set; } | ||
|
|
||
| IExecutionContext FinalizeContext { get; set; } |
There was a problem hiding this comment.
we'll be able to clean this up when the handler performs the steps-runner-ish things
| /// </summary> | ||
| public void RegisterNestedStep(IStep step, DictionaryContextData inputsData, int location, Dictionary<string, string> envData) | ||
| public IStep RegisterNestedStep( | ||
| IActionRunner step, DictionaryContextData inputsData, int location, |
There was a problem hiding this comment.
nit: lets format all on one liner or all separate
| void ForceTaskComplete(); | ||
| void RegisterPostJobStep(IStep step); | ||
| void RegisterNestedStep(IStep step, DictionaryContextData inputsData, int location, Dictionary<string, string> envData); | ||
| IStep RegisterNestedStep(IActionRunner step, DictionaryContextData inputsData, int location, Dictionary<string, string> envData, Boolean cleanUp = false); |
|
|
||
| // Set Scope Name. Note, for our design, we consider each step in a composite action to have the same scope | ||
| // This makes it much simpler to handle their outputs at the end of the Composite Action | ||
| var childScopeName = !string.IsNullOrEmpty(this.ScopeName) ? $"{this.ScopeName}.{this.ContextName}" : this.ContextName; |
There was a problem hiding this comment.
i think there's a bug here for the future (nested composite actions)
Should generate __<GUID> only when context name is empty.
There was a problem hiding this comment.
var safeContextName = !string.IsNullOrEmpty(ContextName) ? ContextName : $"__{newGuid}";
var childScopeName = !string.IsNullOrEmpty(this.ScopeName) ? $"{this.ScopeName}.{safeContextName}" : safeContextName;
is this ^ right? i'm getting confused now because childContextName is passed to Root.CreateChild and i dont know what that's for....
OK i just realized I was confused whether inside the for loop yet or not... i'll leave these notes here and will think through again later.
| if (String.IsNullOrEmpty(ContextName)) | ||
| // Checks if scope name is null or | ||
| // if the ScopeName follows the __GUID format which is set as the default value for ScopeNames if null for Composite Actions. | ||
| bool scopeNameCondition = !string.IsNullOrEmpty(Environment.GetEnvironmentVariable("TESTING_COMPOSITE_ACTIONS_ALPHA")) && !String.IsNullOrEmpty(ScopeName) && ScopeName.Length >= 36 && String.Equals(ScopeName.Substring(0, 2), "__") && Guid.TryParse(ScopeName.Substring(2, 36), out Guid test); |
There was a problem hiding this comment.
consider static regex precompiled
There was a problem hiding this comment.
using System.Text.RegularExpressions;
[...]
private readonly static Regex _generatedContextNamePattern = new Regex("^__[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}$", RegexOptions.Compiled | RegexOptions.CultureInvariant | RegexOptions.IgnoreCase);
Then you can do "_generatedContextNamePattern.IsMatch( )"
| // Steps context (StepsRunner manages adding the scoped steps context) | ||
| StepsContext = new StepsContext(); | ||
|
|
||
| // Scopes |
There was a problem hiding this comment.
i love seeing dead code deleted :)
| // Add each composite action step to the front of the queue | ||
| int location = 0; | ||
|
|
||
| Dictionary<string, string> scopesAndContexts = new Dictionary<string, string>(); |
There was a problem hiding this comment.
this local variable isnt used
| var outputsValue = val as StringContextData; | ||
|
|
||
| // Set output in the whole composite scope. | ||
| if (!String.IsNullOrEmpty(outputsName) && !String.IsNullOrEmpty(outputsValue)) |
There was a problem hiding this comment.
nit: string not String
| { | ||
| // Populate env context for each step | ||
| Trace.Info("Initialize Env context for step"); | ||
| step.ExecutionContext.ExpressionValues["steps"] = step.ExecutionContext.StepsContext.GetScope(step.ExecutionContext.ScopeName); |
There was a problem hiding this comment.
What's different about this file?
if (InitializeScope[...] was removed and the different indent-level is causing too much red/green. I can't tell what changed.
I see the InitializeScope method was deleted. Anything else?
There was a problem hiding this comment.
@ericsciple InitializeScope was completely removed.
Also, most of the CompleteStep was removed so that it looks like. All of the code removed related to the "Scopes" attribute of the ExecutionContext which is no longer used afaik:
private void CompleteStep(IStep step, TaskResult? result = null, string resultCode = null)
{
var executionContext = step.ExecutionContext;
executionContext.Complete(result, resultCode: resultCode);
}
| return result; | ||
| } | ||
|
|
||
| public DictionaryContextData EvaluateCompositeOutputs( |
There was a problem hiding this comment.
This should be in ActionManifestManager.cs instead. For example see EvaluateDefaultInput
| public const String Clean = "clean"; | ||
| public const String Container = "container"; | ||
| public const String ContinueOnError = "continue-on-error"; | ||
| public const String CompositeOutputs = "composite-outputs"; |
There was a problem hiding this comment.
revert this file (see comment on PipelineTemplateEvaluator)
There was a problem hiding this comment.
+1, otherwise you will need to port this back to server code
| // Evaluate the outputs in the steps context to easily retrieve the values | ||
| DictionaryContextData actionOutputs = evaluator.EvaluateCompositeOutputs(Data.Outputs, ExecutionContext.ExpressionValues, ExecutionContext.ExpressionFunctions); | ||
|
|
||
| // Each pair is structured like this |
There was a problem hiding this comment.
Since comment intended for conceptual understanding of the structure, and not how the object is serialized, consider plain json dictionaries to describe the structure instead
{
"the-output-name": {
"description": "",
"value": "the value"
},
...
}
src/Runner.Worker/action_yaml.json
Outdated
| @@ -1,4 +1,6 @@ | |||
| { | |||
| "version": "action_v1.0", | |||
There was a problem hiding this comment.
do we need to version this file for now?
| var evaluator = new Pipelines.ObjectTemplating.PipelineTemplateEvaluator(ExecutionContext.ToTemplateTraceWriter(), ExecutionContext.ActionManifestSchema, ExecutionContext.FinalizeContext.FileTable); | ||
|
|
||
| // Evaluate the outputs in the steps context to easily retrieve the values | ||
| DictionaryContextData actionOutputs = evaluator.EvaluateCompositeOutputs(Data.Outputs, ExecutionContext.ExpressionValues, ExecutionContext.ExpressionFunctions); |
There was a problem hiding this comment.
Consider moving this to ActionManifestManager
| // Set output in the whole composite scope. | ||
| if (!String.IsNullOrEmpty(outputsName) && !String.IsNullOrEmpty(outputsValue)) | ||
| { | ||
| ExecutionContext.FinalizeContext.SetOutput(outputsName, outputsValue, out string test); |
There was a problem hiding this comment.
use discards: ExecutionContext.FinalizeContext.SetOutput(outputsName, outputsValue, out _);
src/Runner.Worker/action_yaml.json
Outdated
| "inputs": "inputs", | ||
| "runs": "runs" | ||
| "runs": "runs", | ||
| "outputs": "composite-outputs" |
There was a problem hiding this comment.
is composite-outputs or just outputs even it only matter for composite actions for now?
| { | ||
| public CompositeActionExecutionData Data { get; set; } | ||
|
|
||
| private void InitializeScope(IStep step) |
There was a problem hiding this comment.
move private method down after public method
* 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
* Initial start/framework for output handling
* Outline different class vs Handler approach
* Remove InitializeScope
* Remove InitializeScope
* Fix Workflow Step Env overiding Parent Env
* First Approach for Attaching ID + Group ID to each Composite Action Step
* Add GroupID to the ActionDefinitionData
* starting foundation for handling clean up outputs step
* Pass outputs data to each composite action step to enable set-output functionality
* Create ScopeName for whole composite action.
This will enable us to add to the StepsContext[ScopeName] for the composite action which will allow us to use all these outputs in the cleanup step
* Hook up composite output step to handler => tmmrw implement composite output handler
* Add post composite action step to cleanup outputs => triggers composite output cleanup handler
* Fix Outputs Token handling start. Add individual step scope names.
* Set up Scope Name and Context Name naming system{
* Figured out how to pass Parent Execution Context to clean up step
* Figured out how to pass Parent Execution Context and scope names to
clean up step
* Add GetOutput function for StepsContext
* Generate child scope name correctly if parent scope name is null
* Simplify InitializeScope()
* Outputs are set correctly and able to get all final outputs in handler
* Parse through Action Outputs
* Fix null ScopeName + ContextName in CompositeOutputHandler
* Shift over handling of Action Outputs to output handler
* First attempt to fix null retrievals for output variables
* Basic Support for Outputs Done.
* Clean up pt.1
* Refactor outputs to avoid using Action Reference
* Clean up code
* Clean up part 2
* Add clarifying comments for the output handler
* Remove TODO
* 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
* Revert
* Revert
* revert
* spacing
* Add filetable to testing file, remove ? since we know filetable should never be non null
* Fix Throw logic
* Clarify template outputs token
* Add another type support for outputs to avoid container unit tests errors
* Add mapping for parity
* Build support for new outputs format
* Refactor to avoid duplication of action yaml for workflow yaml
* Move SDK work in ActionManifestManager, Condense Code
* Defer runs evaluation till after for loop to ensure order doesn't matter
* Fix logic error in setting scope and context names
* Add Regex + Add Child Context name null resolution
* move private function to bottom of class
In this PR, I add support for handling Outputs as described in https://github.com/actions/runner/blob/users/ethanchewy/compositeADR/docs/adrs/0549-composite-run-steps.md#outputs Throughout this process, we remove unnecessary old code relating to the scope.
Demo: https://github.com/ethanchewy/testing-actions/runs/855647900?check_suite_focus=true
action.yml
workflow.yml
Goal: Handle Composite Run steps.
Pretty large in scope.
To achieve this, we need to do:
set-outputin steps since there were no steps. We need scope name for creating the right scopes for setting output variables.outputstoken in action yaml file::set-output::TODO 7/6/20:
Refactoring Info for those who are curious:
We return the same outputs execution data type as the Composite Action Output Handler
In the Composite Action Output Handler, we refactor so that we evaluate the scope outputs in the same context as the StepsContext. Then, we go up a scope and set the outputs in the whole scope of the whole Composite Action.