Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Composite Run Steps Refactoring #591

Merged
merged 52 commits into from
Jul 17, 2020
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
52 commits
Select commit Hold shift + click to select a range
3251ead
Add basic framework for baby steps runner
ethanchewy Jul 13, 2020
06adbae
Basic logic for adding steps / invoking composite action steps
ethanchewy Jul 14, 2020
9fc6f80
Composite Steps Runner MVP
ethanchewy Jul 14, 2020
50d4f9d
Fix null object reference error
ethanchewy Jul 14, 2020
bb84921
intialize composiute
ethanchewy Jul 14, 2020
eb0e456
Comment out code that is handled by stepsrunner
ethanchewy Jul 14, 2020
c63709d
Add composite clean up step
ethanchewy Jul 14, 2020
2067778
Remove previous 'workarounds' from StepsRunner. Clean Up PR
ethanchewy Jul 15, 2020
96ac86f
Remove todo
ethanchewy Jul 15, 2020
f322697
Remove todo
ethanchewy Jul 15, 2020
2c157b4
Fix using unitialized object yikes
ethanchewy Jul 15, 2020
256e455
Remove time delay
ethanchewy Jul 15, 2020
e42a7c3
Format handler
ethanchewy Jul 15, 2020
49ba1c8
Move output handler into action handler
ethanchewy Jul 15, 2020
45639a2
Add try to evaluate display name
ethanchewy Jul 15, 2020
87d3030
Remove while loop yikes
ethanchewy Jul 15, 2020
e0561ad
Abstract away the windows encoding check during step running
ethanchewy Jul 15, 2020
25bd4b1
Github context set to {ScopeName}.{ContextName} or {ContextName} if S…
ethanchewy Jul 15, 2020
f5680aa
Remove setting result to sucess since result defaults to sucess
ethanchewy Jul 15, 2020
74ba0eb
Fix windows error
ethanchewy Jul 15, 2020
8bf57f7
Fix windows
ethanchewy Jul 16, 2020
b0d92bb
revert:
ethanchewy Jul 16, 2020
309e09b
Windows fix
ethanchewy Jul 16, 2020
7be28ab
Fix Windows Error in Abstraction
ethanchewy Jul 16, 2020
7d95fa6
Remove Composite Steps Runner => consolidate into Composite Steps Runner
ethanchewy Jul 16, 2020
16df485
Remove unn. attribute in ExecutionContext
ethanchewy Jul 16, 2020
79e572f
Change protection levels, plus change function name to more clear mea…
ethanchewy Jul 16, 2020
ce52070
Remove location param
ethanchewy Jul 16, 2020
dc36e2e
location pt.2 fix
ethanchewy Jul 16, 2020
3d4f7bd
Remove outputs step
ethanchewy Jul 16, 2020
bc613a0
Remove temp directory
ethanchewy Jul 16, 2020
a0af7d6
new line
ethanchewy Jul 16, 2020
81bdee6
Add arguitl not null
ethanchewy Jul 16, 2020
44ed247
better comment
ethanchewy Jul 16, 2020
3c2d8f1
Change encoding name
ethanchewy Jul 16, 2020
868a42a
Check count > 0 for composite steps, import System.Threading
ethanchewy Jul 16, 2020
61a0049
Change function header encodingutil
ethanchewy Jul 16, 2020
df4e4c1
Add TODO
ethanchewy Jul 16, 2020
1cafbf0
Add await
ethanchewy Jul 16, 2020
470708d
Handle Failed Step
ethanchewy Jul 16, 2020
3915791
Move over SetAllCompositeOutputs to the handler
ethanchewy Jul 16, 2020
a40c07b
Remove timeout-minutes setting in steps-level
ethanchewy Jul 16, 2020
7d96e5d
Use only ExecutionContext
ethanchewy Jul 17, 2020
8714315
Move using to the top
ethanchewy Jul 17, 2020
ca79af5
Remove redundant check
ethanchewy Jul 17, 2020
b6c3091
Change function name
ethanchewy Jul 17, 2020
22c4cb9
Remove testing code
ethanchewy Jul 17, 2020
89002ce
Consolidate error code
ethanchewy Jul 17, 2020
0ad860e
Consolidate code
ethanchewy Jul 17, 2020
28db0aa
Change HandleOutput => ProcessCompositeActionOutputs
ethanchewy Jul 17, 2020
dd476b8
Remove set the timeout comment
ethanchewy Jul 17, 2020
d9e5109
Add Cancelling functionality + Remove unn. parameter
ethanchewy Jul 17, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
249 changes: 249 additions & 0 deletions src/Runner.Worker/CompositeStepsRunner.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,249 @@
using System;
using System.Collections.Generic;
using System.Text;
using System.Threading;
using System.Threading.Tasks;
using GitHub.DistributedTask.Expressions2;
using GitHub.DistributedTask.ObjectTemplating.Tokens;
using GitHub.DistributedTask.Pipelines;
using GitHub.DistributedTask.Pipelines.ContextData;
using GitHub.DistributedTask.Pipelines.ObjectTemplating;
using GitHub.DistributedTask.WebApi;
using GitHub.Runner.Common;
using GitHub.Runner.Common.Util;
using GitHub.Runner.Sdk;
using GitHub.Runner.Worker.Expressions;
using ObjectTemplating = GitHub.DistributedTask.ObjectTemplating;
using Pipelines = GitHub.DistributedTask.Pipelines;

namespace GitHub.Runner.Worker
{
[ServiceLocator(Default = typeof(CompositeStepsRunner))]
public interface ICompositeStepsRunner : IRunnerService
{
Task RunAsync(IExecutionContext Context);
}


public sealed class CompositeStepsRunner : RunnerService, ICompositeStepsRunner
{
public async Task RunAsync(IExecutionContext actionContext)
{
// Another approach we can explore, is moving all this logic to the CompositeActionHandler if it's small enough.
ethanchewy marked this conversation as resolved.
Show resolved Hide resolved

ArgUtil.NotNull(actionContext, nameof(actionContext));
ArgUtil.NotNull(actionContext.CompositeSteps, nameof(actionContext.CompositeSteps));

// The parent StepsRunner of the whole Composite Action Step handles the cancellation stuff already.
while (actionContext.CompositeSteps.Count > 0)
ethanchewy marked this conversation as resolved.
Show resolved Hide resolved
{
// This is used for testing UI appearance.
// System.Threading.Thread.Sleep(5000);

var step = actionContext.CompositeSteps[0];
actionContext.CompositeSteps.RemoveAt(0);

Trace.Info($"Processing composite step: DisplayName='{step.DisplayName}'");

step.ExecutionContext.ExpressionValues["steps"] = step.ExecutionContext.StepsContext.GetScope(step.ExecutionContext.ScopeName);

// Populate env context for each step
Trace.Info("Initialize Env context for step");
#if OS_WINDOWS
var envContext = new DictionaryContextData();
#else
var envContext = new CaseSensitiveDictionaryContextData();
#endif

// Global env
foreach (var pair in step.ExecutionContext.EnvironmentVariables)
{
envContext[pair.Key] = new StringContextData(pair.Value ?? string.Empty);
}

// Stomps over with outside step env
if (step.ExecutionContext.ExpressionValues.TryGetValue("env", out var envContextData))
{
#if OS_WINDOWS
var dict = envContextData as DictionaryContextData;
#else
var dict = envContextData as CaseSensitiveDictionaryContextData;
#endif
foreach (var pair in dict)
{
envContext[pair.Key] = pair.Value;
}
}

step.ExecutionContext.ExpressionValues["env"] = envContext;



ethanchewy marked this conversation as resolved.
Show resolved Hide resolved
if (step is IActionRunner actionStep)
ethanchewy marked this conversation as resolved.
Show resolved Hide resolved
{
// Set GITHUB_ACTION
step.ExecutionContext.SetGitHubContext("action", actionStep.Action.Name);
ethanchewy marked this conversation as resolved.
Show resolved Hide resolved

try
{
// Evaluate and merge action's env block to env context
var templateEvaluator = step.ExecutionContext.ToPipelineTemplateEvaluator();
var actionEnvironment = templateEvaluator.EvaluateStepEnvironment(actionStep.Action.Environment, step.ExecutionContext.ExpressionValues, step.ExecutionContext.ExpressionFunctions, VarUtil.EnvironmentVariableKeyComparer);
foreach (var env in actionEnvironment)
{
envContext[env.Key] = new StringContextData(env.Value ?? string.Empty);
}
}
catch (Exception ex)
{
// fail the step since there is an evaluate error.
Trace.Info("Caught exception in Composite Steps Runner from expression for step.env");
// evaluateStepEnvFailed = true;
step.ExecutionContext.Error(ex);
// CompleteStep(step, TaskResult.Failed);
ethanchewy marked this conversation as resolved.
Show resolved Hide resolved
}
}

// We don't have to worry about the cancellation token stuff because that's handled by the composite action level (in the StepsRunner)

await RunStepAsync(step);

// TODO: Add compat for other types of steps.
}
// Completion Status handled by StepsRunner for the whole Composite Action Step
}

private async Task RunStepAsync(IStep step)
{
// Check to see if we can expand the display name
if (step is IActionRunner actionRunner &&
actionRunner.Stage == ActionRunStage.Main &&
actionRunner.TryEvaluateDisplayName(step.ExecutionContext.ExpressionValues, step.ExecutionContext))
{
step.ExecutionContext.UpdateTimelineRecordDisplayName(actionRunner.DisplayName);
ethanchewy marked this conversation as resolved.
Show resolved Hide resolved
}

// Start the step.
Trace.Info("Starting the step.");
step.ExecutionContext.Debug($"Starting: {step.DisplayName}");

// Set the timeout
var timeoutMinutes = 0;
var templateEvaluator = step.ExecutionContext.ToPipelineTemplateEvaluator();
try
{
timeoutMinutes = templateEvaluator.EvaluateStepTimeout(step.Timeout, step.ExecutionContext.ExpressionValues, step.ExecutionContext.ExpressionFunctions);
}
catch (Exception ex)
{
Trace.Info("An error occurred when attempting to determine the step timeout.");
Trace.Error(ex);
step.ExecutionContext.Error("An error occurred when attempting to determine the step timeout.");
step.ExecutionContext.Error(ex);
}
if (timeoutMinutes > 0)
{
var timeout = TimeSpan.FromMinutes(timeoutMinutes);
step.ExecutionContext.SetTimeout(timeout);
}

#if OS_WINDOWS
try
ethanchewy marked this conversation as resolved.
Show resolved Hide resolved
{
if (Console.InputEncoding.CodePage != 65001)
{
using (var p = HostContext.CreateService<IProcessInvoker>())
{
// Use UTF8 code page
int exitCode = await p.ExecuteAsync(workingDirectory: HostContext.GetDirectory(WellKnownDirectory.Work),
fileName: WhichUtil.Which("chcp", true, Trace),
arguments: "65001",
environment: null,
requireExitCodeZero: false,
outputEncoding: null,
killProcessOnCancel: false,
redirectStandardIn: null,
inheritConsoleHandler: true,
cancellationToken: step.ExecutionContext.CancellationToken);
if (exitCode == 0)
{
Trace.Info("Successfully returned to code page 65001 (UTF8)");
}
else
{
Trace.Warning($"'chcp 65001' failed with exit code {exitCode}");
}
}
}
}
catch (Exception ex)
{
Trace.Warning($"'chcp 65001' failed with exception {ex.Message}");
}
#endif

try
{
await step.RunAsync();
}
catch (OperationCanceledException ex)
{
if (step.ExecutionContext.CancellationToken.IsCancellationRequested)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to distinguish between this step timing-out vs the outer step timing-out?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think timeout will work properly right now, since the step.ExecutionContext.SetTimeout(timeout); is called on the StepRunner for the whole composite action

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a TODO so that I remember to do this for timeout-minutes

{
Trace.Error($"Caught timeout exception from step: {ex.Message}");
step.ExecutionContext.Error("The action has timed out.");
step.ExecutionContext.Result = TaskResult.Failed;
}
else
{
// Log the exception and cancel the step.
Trace.Error($"Caught cancellation exception from step: {ex}");
step.ExecutionContext.Error(ex);
step.ExecutionContext.Result = TaskResult.Canceled;
}
}
catch (Exception ex)
{
// Log the error and fail the step.
Trace.Error($"Caught exception from step: {ex}");
step.ExecutionContext.Error(ex);
step.ExecutionContext.Result = TaskResult.Failed;
}

// Merge execution context result with command result
if (step.ExecutionContext.CommandResult != null)
{
step.ExecutionContext.Result = TaskResultUtil.MergeTaskResults(step.ExecutionContext.Result, step.ExecutionContext.CommandResult.Value);
}

// Fixup the step result if ContinueOnError.
if (step.ExecutionContext.Result == TaskResult.Failed)
{
var continueOnError = false;
try
{
continueOnError = templateEvaluator.EvaluateStepContinueOnError(step.ContinueOnError, step.ExecutionContext.ExpressionValues, step.ExecutionContext.ExpressionFunctions);
}
catch (Exception ex)
{
Trace.Info("The step failed and an error occurred when attempting to determine whether to continue on error.");
Trace.Error(ex);
step.ExecutionContext.Error("The step failed and an error occurred when attempting to determine whether to continue on error.");
step.ExecutionContext.Error(ex);
}

if (continueOnError)
{
step.ExecutionContext.Outcome = step.ExecutionContext.Result;
step.ExecutionContext.Result = TaskResult.Succeeded;
Trace.Info($"Updated step result (continue on error)");
}
}
Trace.Info($"Step result: {step.ExecutionContext.Result}");

// Complete the step context.
step.ExecutionContext.Debug($"Finishing: {step.DisplayName}");

ethanchewy marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
10 changes: 7 additions & 3 deletions src/Runner.Worker/ExecutionContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,9 @@ public interface IExecutionContext : IRunnerService

IExecutionContext FinalizeContext { get; set; }

// Only Composite Level ExecutionContext has CompositeSteps (public setter for handler)
List<IStep> CompositeSteps { get; set; }
ethanchewy marked this conversation as resolved.
Show resolved Hide resolved

// Initialize
void InitializeJob(Pipelines.AgentJobRequestMessage message, CancellationToken token);
void CancelToken();
Expand Down Expand Up @@ -172,6 +175,9 @@ public sealed class ExecutionContext : RunnerService, IExecutionContext
// Only job level ExecutionContext has StepsWithPostRegistered
public HashSet<Guid> StepsWithPostRegistered { get; private set; }

// Only Composite Level ExecutionContext has CompositeSteps (public setter for handler)
public List<IStep> CompositeSteps { get; set; }

public bool EchoOnActionCommand { get; set; }

public IExecutionContext FinalizeContext { get; set; }
Expand Down Expand Up @@ -314,9 +320,7 @@ public IStep RegisterNestedStep(
envContext[pair.Key] = new StringContextData(pair.Value ?? string.Empty);
}
step.ExecutionContext.ExpressionValues["env"] = envContext;

Root.JobSteps.Insert(location, step);


return step;
}

Expand Down
30 changes: 26 additions & 4 deletions src/Runner.Worker/Handlers/CompositeActionHandler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ public sealed class CompositeActionHandler : Handler, ICompositeActionHandler
{
public CompositeActionExecutionData Data { get; set; }

public Task RunAsync(ActionRunStage stage)
public async Task RunAsync(ActionRunStage stage)
ethanchewy marked this conversation as resolved.
Show resolved Hide resolved
{
// Validate args.
Trace.Entering();
Expand All @@ -48,6 +48,12 @@ public Task RunAsync(ActionRunStage stage)
// Add each composite action step to the front of the queue
int location = 0;
ethanchewy marked this conversation as resolved.
Show resolved Hide resolved

// Initialize Steps Runner for Composite Steps and Environment for it
var compositeStepsRunner = HostContext.CreateService<ICompositeStepsRunner>();

// Initialize Composite Steps List of Steps
ExecutionContext.CompositeSteps = new List<IStep>();

foreach (Pipelines.ActionStep aStep in actionSteps)
{
// Ex:
Expand Down Expand Up @@ -86,9 +92,12 @@ public Task RunAsync(ActionRunStage stage)
actionRunner.Condition = aStep.Condition;

var step = ExecutionContext.RegisterNestedStep(actionRunner, inputsData, location, Environment);

InitializeScope(step);

// Add all steps to the Composite StepsRunner
// Follows similar logic to how JobRunner invokes the StepsRunner for job steps!
ethanchewy marked this conversation as resolved.
Show resolved Hide resolved
ExecutionContext.CompositeSteps.Add(step);

location++;
}

Expand All @@ -102,9 +111,22 @@ public Task RunAsync(ActionRunStage stage)
actionRunner2.Action = cleanOutputsStep;
actionRunner2.Stage = ActionRunStage.Main;
actionRunner2.Condition = "always()";
ExecutionContext.RegisterNestedStep(actionRunner2, inputsData, location, Environment, true);
var cleanUpStep = ExecutionContext.RegisterNestedStep(actionRunner2, inputsData, location, Environment, true);
ethanchewy marked this conversation as resolved.
Show resolved Hide resolved
ExecutionContext.CompositeSteps.Add(cleanUpStep);

return Task.CompletedTask;
// Then run the Composite StepsRunner
try
{
await compositeStepsRunner.RunAsync(ExecutionContext);
}
catch (Exception ex)
{
// Composite StepRunner should never throw exception out.
Trace.Error($"Caught exception from composite steps {nameof(CompositeStepsRunner)}: {ex}");
ExecutionContext.Error(ex);
ExecutionContext.Result = TaskResult.Failed;
}
ExecutionContext.Result = TaskResult.Succeeded;
ethanchewy marked this conversation as resolved.
Show resolved Hide resolved
}

private void InitializeScope(IStep step)
Expand Down
15 changes: 0 additions & 15 deletions src/Runner.Worker/StepsRunner.cs
Original file line number Diff line number Diff line change
Expand Up @@ -92,26 +92,11 @@ public async Task RunAsync(IExecutionContext jobContext)
var envContext = new CaseSensitiveDictionaryContextData();
#endif

// Global env
foreach (var pair in step.ExecutionContext.EnvironmentVariables)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes are to revert the workaround we used to flow Env to composite actions. This is now handled in the Composite Steps Runner.

{
envContext[pair.Key] = new StringContextData(pair.Value ?? string.Empty);
}

// Stomps over with outside step env
if (step.ExecutionContext.ExpressionValues.TryGetValue("env", out var envContextData))
{
#if OS_WINDOWS
var dict = envContextData as DictionaryContextData;
#else
var dict = envContextData as CaseSensitiveDictionaryContextData;
#endif
foreach (var pair in dict)
{
envContext[pair.Key] = pair.Value;
}
}

step.ExecutionContext.ExpressionValues["env"] = envContext;

bool evaluateStepEnvFailed = false;
Expand Down