From 9939cf527eaadece243d0c5c75e78de478f1c556 Mon Sep 17 00:00:00 2001 From: Ethan Chiu Date: Fri, 19 Jun 2020 14:29:48 -0400 Subject: [PATCH 1/4] Change JobSteps to a List, Change Register Step function name --- src/Runner.Worker/ExecutionContext.cs | 68 +++---------------- .../Handlers/CompositeActionHandler.cs | 8 +-- src/Runner.Worker/JobRunner.cs | 2 +- src/Runner.Worker/StepsRunner.cs | 7 +- 4 files changed, 20 insertions(+), 65 deletions(-) diff --git a/src/Runner.Worker/ExecutionContext.cs b/src/Runner.Worker/ExecutionContext.cs index b99ff99cd..228074e6c 100644 --- a/src/Runner.Worker/ExecutionContext.cs +++ b/src/Runner.Worker/ExecutionContext.cs @@ -63,7 +63,7 @@ namespace GitHub.Runner.Worker JobContext JobContext { get; } // Only job level ExecutionContext has JobSteps - Queue JobSteps { get; } + List JobSteps { get; } // Only job level ExecutionContext has PostJobSteps Stack PostJobSteps { get; } @@ -105,8 +105,7 @@ namespace GitHub.Runner.Worker // others void ForceTaskComplete(); void RegisterPostJobStep(IStep step); - IStep RegisterCompositeStep(IStep step, DictionaryContextData inputsData); - void EnqueueAllCompositeSteps(Queue steps); + void RegisterNestedStep(IStep step, DictionaryContextData inputsData, int location); } public sealed class ExecutionContext : RunnerService, IExecutionContext @@ -161,7 +160,7 @@ namespace GitHub.Runner.Worker public List ServiceContainers { get; private set; } // Only job level ExecutionContext has JobSteps - public Queue JobSteps { get; private set; } + public List JobSteps { get; private set; } // Only job level ExecutionContext has PostJobSteps public Stack PostJobSteps { get; private set; } @@ -267,62 +266,17 @@ namespace GitHub.Runner.Worker Root.PostJobSteps.Push(step); } - /* - RegisterCompositeStep is a helper function used in CompositeActionHandler::RunAsync to - add a child node, aka a step, to the current job to the front of the queue for processing. - */ - public IStep RegisterCompositeStep(IStep step, DictionaryContextData inputsData) + /// + /// Helper function used in CompositeActionHandler::RunAsync to + /// add a child node, aka a step, to the current job to the Root.JobSteps based on the location. + /// + public void RegisterNestedStep(IStep step, DictionaryContextData inputsData, int location) { - // ~Brute Force Method~ - // There is no way to put this current job in front of the queue in < O(n) time where n = # of elements in JobSteps - // Everytime we add a new step, you could requeue every item to put those steps from that stack in JobSteps which - // would result in O(n) for each time we add a composite action step where n = number of jobSteps which would compound - // to O(n*m) where m = number of composite steps - // var temp = Root.JobSteps.ToArray(); - // Root.JobSteps.Clear(); - // Root.JobSteps.Enqueue(step); - // foreach(var s in temp) - // Root.JobSteps.Enqueue(s); - - // ~Optimized Method~ - // Alterative solution: We add to another temp Queue - // After we add all the transformed composite steps to this temp queue, we requeue the whole JobSteps accordingly in EnqueueAllCompositeSteps() - // where the queued composite steps are at the front of the JobSteps Queue and the rest of the jobs maintain its order and are - // placed after the queued composite steps - // This will take only O(n+m) time which would be just as efficient if we refactored the code of JobSteps to a PriorityQueue - // This temp Queue is created in the CompositeActionHandler. + // TODO: For UI purposes, look at figuring out how to condense steps in one node => maybe use the same previous GUID var newGuid = Guid.NewGuid(); step.ExecutionContext = Root.CreateChild(newGuid, step.DisplayName, newGuid.ToString("N"), null, null); step.ExecutionContext.ExpressionValues["inputs"] = inputsData; - return step; - } - - // Add Composite Steps first and then requeue the rest of the job steps. - public void EnqueueAllCompositeSteps(Queue steps) - { - // TODO: For UI purposes, look at figuring out how to condense steps in one node - // maybe use "this" instead of "Root"? - if (Root.JobSteps != null) - { - var temp = Root.JobSteps.ToArray(); - Root.JobSteps.Clear(); - foreach (var cs in steps) - { - Root.JobSteps.Enqueue(cs); - } - foreach (var s in temp) - { - Root.JobSteps.Enqueue(s); - } - } - else - { - Root.JobSteps = new Queue(); - foreach (var cs in steps) - { - Root.JobSteps.Enqueue(cs); - } - } + Root.JobSteps.Insert(0, step); } public IExecutionContext CreateChild(Guid recordId, string displayName, string refName, string scopeName, string contextName, Dictionary intraActionState = null, int? recordOrder = null) @@ -719,7 +673,7 @@ namespace GitHub.Runner.Worker PrependPath = new List(); // JobSteps for job ExecutionContext - JobSteps = new Queue(); + JobSteps = new List(); // PostJobSteps for job ExecutionContext PostJobSteps = new Stack(); diff --git a/src/Runner.Worker/Handlers/CompositeActionHandler.cs b/src/Runner.Worker/Handlers/CompositeActionHandler.cs index c29e6c10f..74c3d4c7e 100644 --- a/src/Runner.Worker/Handlers/CompositeActionHandler.cs +++ b/src/Runner.Worker/Handlers/CompositeActionHandler.cs @@ -45,7 +45,7 @@ namespace GitHub.Runner.Worker.Handlers } // Add each composite action step to the front of the queue - var compositeActionSteps = new Queue(); + int location = 0; foreach (Pipelines.ActionStep aStep in actionSteps) { // Ex: @@ -63,7 +63,7 @@ namespace GitHub.Runner.Worker.Handlers // - run echo hello world 3 (d) // - run echo hello world 4 (e) // - // Stack (LIFO) [Bottom => Middle => Top]: + // Steps processed as follow: // | a | // | a | => | d | // (Run step d) @@ -86,9 +86,9 @@ namespace GitHub.Runner.Worker.Handlers // TODO: Do we need to add any context data from the job message? // (See JobExtension.cs ~line 236) - compositeActionSteps.Enqueue(ExecutionContext.RegisterCompositeStep(actionRunner, inputsData)); + ExecutionContext.RegisterNestedStep(actionRunner, inputsData, location); + location++; } - ExecutionContext.EnqueueAllCompositeSteps(compositeActionSteps); return Task.CompletedTask; } diff --git a/src/Runner.Worker/JobRunner.cs b/src/Runner.Worker/JobRunner.cs index 31dfb1714..33b291adb 100644 --- a/src/Runner.Worker/JobRunner.cs +++ b/src/Runner.Worker/JobRunner.cs @@ -152,7 +152,7 @@ namespace GitHub.Runner.Worker { foreach (var step in jobSteps) { - jobContext.JobSteps.Enqueue(step); + jobContext.JobSteps.Add(step); } await stepsRunner.RunAsync(jobContext); diff --git a/src/Runner.Worker/StepsRunner.cs b/src/Runner.Worker/StepsRunner.cs index 3c7b38d4a..9fc323c86 100644 --- a/src/Runner.Worker/StepsRunner.cs +++ b/src/Runner.Worker/StepsRunner.cs @@ -59,14 +59,15 @@ namespace GitHub.Runner.Worker checkPostJobActions = true; while (jobContext.PostJobSteps.TryPop(out var postStep)) { - jobContext.JobSteps.Enqueue(postStep); + jobContext.JobSteps.Add(postStep); } continue; } - var step = jobContext.JobSteps.Dequeue(); - var nextStep = jobContext.JobSteps.Count > 0 ? jobContext.JobSteps.Peek() : null; + var step = jobContext.JobSteps[0]; + jobContext.JobSteps.RemoveAt(0); + var nextStep = jobContext.JobSteps.Count > 0 ? jobContext.JobSteps[0] : null; // TODO: Fix this temporary workaround for Composite Actions if (!string.IsNullOrEmpty(Environment.GetEnvironmentVariable("TESTING_COMPOSITE_ACTIONS_ALPHA"))) { From 5988076fcf9913d98d745549089e85d11a46be8f Mon Sep 17 00:00:00 2001 From: Ethan Chiu Date: Fri, 19 Jun 2020 14:38:37 -0400 Subject: [PATCH 2/4] Add TODO, remove unn. content --- src/Runner.Worker/ExecutionContext.cs | 1 + src/Runner.Worker/Handlers/HandlerFactory.cs | 3 --- .../Pipelines/ObjectTemplating/PipelineTemplateConstants.cs | 1 - 3 files changed, 1 insertion(+), 4 deletions(-) diff --git a/src/Runner.Worker/ExecutionContext.cs b/src/Runner.Worker/ExecutionContext.cs index 228074e6c..67ef66cb7 100644 --- a/src/Runner.Worker/ExecutionContext.cs +++ b/src/Runner.Worker/ExecutionContext.cs @@ -276,6 +276,7 @@ namespace GitHub.Runner.Worker var newGuid = Guid.NewGuid(); step.ExecutionContext = Root.CreateChild(newGuid, step.DisplayName, newGuid.ToString("N"), null, null); step.ExecutionContext.ExpressionValues["inputs"] = inputsData; + // TODO: confirm whether not copying message contexts is safe Root.JobSteps.Insert(0, step); } diff --git a/src/Runner.Worker/Handlers/HandlerFactory.cs b/src/Runner.Worker/Handlers/HandlerFactory.cs index 99311afca..db4d6559c 100644 --- a/src/Runner.Worker/Handlers/HandlerFactory.cs +++ b/src/Runner.Worker/Handlers/HandlerFactory.cs @@ -68,10 +68,7 @@ namespace GitHub.Runner.Worker.Handlers } else if (data.ExecutionType == ActionExecutionType.Composite) { - // TODO - // Runner plugin handler = HostContext.CreateService(); - // handler = CompositeHandler; (handler as ICompositeActionHandler).Data = data as CompositeActionExecutionData; } else diff --git a/src/Sdk/DTPipelines/Pipelines/ObjectTemplating/PipelineTemplateConstants.cs b/src/Sdk/DTPipelines/Pipelines/ObjectTemplating/PipelineTemplateConstants.cs index bf7d8d2e9..f2609462b 100644 --- a/src/Sdk/DTPipelines/Pipelines/ObjectTemplating/PipelineTemplateConstants.cs +++ b/src/Sdk/DTPipelines/Pipelines/ObjectTemplating/PipelineTemplateConstants.cs @@ -65,7 +65,6 @@ namespace GitHub.DistributedTask.Pipelines.ObjectTemplating public const String StepEnv = "step-env"; public const String StepIfResult = "step-if-result"; public const String Steps = "steps"; - public const String StepsInTemplate = "steps-in-template"; public const String StepsScopeInputs = "steps-scope-inputs"; public const String StepsScopeOutputs = "steps-scope-outputs"; From 00a736d9cc5e3fe6e2c9b9776d8915c9edc18b51 Mon Sep 17 00:00:00 2001 From: Ethan Chiu Date: Fri, 19 Jun 2020 14:43:31 -0400 Subject: [PATCH 3/4] Remove unnecessary code --- src/Runner.Worker/StepsRunner.cs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/Runner.Worker/StepsRunner.cs b/src/Runner.Worker/StepsRunner.cs index 9fc323c86..e75d2e106 100644 --- a/src/Runner.Worker/StepsRunner.cs +++ b/src/Runner.Worker/StepsRunner.cs @@ -68,11 +68,6 @@ namespace GitHub.Runner.Worker var step = jobContext.JobSteps[0]; jobContext.JobSteps.RemoveAt(0); var nextStep = jobContext.JobSteps.Count > 0 ? jobContext.JobSteps[0] : null; - // TODO: Fix this temporary workaround for Composite Actions - if (!string.IsNullOrEmpty(Environment.GetEnvironmentVariable("TESTING_COMPOSITE_ACTIONS_ALPHA"))) - { - nextStep = null; - } Trace.Info($"Processing step: DisplayName='{step.DisplayName}'"); ArgUtil.NotNull(step.ExecutionContext, nameof(step.ExecutionContext)); From e4dfd0e8fd39bfa5592ea5e517790b0f33fe39bd Mon Sep 17 00:00:00 2001 From: Ethan Chiu Date: Fri, 19 Jun 2020 14:51:16 -0400 Subject: [PATCH 4/4] Fix unit tests --- src/Test/L0/Worker/StepsRunnerL0.cs | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/Test/L0/Worker/StepsRunnerL0.cs b/src/Test/L0/Worker/StepsRunnerL0.cs index 2d7cb9fb0..1dfee2252 100644 --- a/src/Test/L0/Worker/StepsRunnerL0.cs +++ b/src/Test/L0/Worker/StepsRunnerL0.cs @@ -80,7 +80,7 @@ namespace GitHub.Runner.Common.Tests.Worker { _ec.Object.Result = null; - _ec.Setup(x => x.JobSteps).Returns(new Queue(variableSet.Select(x => x.Object).ToList())); + _ec.Setup(x => x.JobSteps).Returns(new List(variableSet.Select(x => x.Object).ToList())); // Act. await _stepsRunner.RunAsync(jobContext: _ec.Object); @@ -115,7 +115,7 @@ namespace GitHub.Runner.Common.Tests.Worker { _ec.Object.Result = null; - _ec.Setup(x => x.JobSteps).Returns(new Queue(variableSet.Select(x => x.Object).ToList())); + _ec.Setup(x => x.JobSteps).Returns(new List(variableSet.Select(x => x.Object).ToList())); // Act. await _stepsRunner.RunAsync(jobContext: _ec.Object); @@ -154,7 +154,7 @@ namespace GitHub.Runner.Common.Tests.Worker { _ec.Object.Result = null; - _ec.Setup(x => x.JobSteps).Returns(new Queue(variableSet.Steps.Select(x => x.Object).ToList())); + _ec.Setup(x => x.JobSteps).Returns(new List(variableSet.Steps.Select(x => x.Object).ToList())); // Act. await _stepsRunner.RunAsync(jobContext: _ec.Object); @@ -208,7 +208,7 @@ namespace GitHub.Runner.Common.Tests.Worker { _ec.Object.Result = null; - _ec.Setup(x => x.JobSteps).Returns(new Queue(variableSet.Steps.Select(x => x.Object).ToList())); + _ec.Setup(x => x.JobSteps).Returns(new List(variableSet.Steps.Select(x => x.Object).ToList())); // Act. await _stepsRunner.RunAsync(jobContext: _ec.Object); @@ -287,7 +287,7 @@ namespace GitHub.Runner.Common.Tests.Worker { _ec.Object.Result = null; - _ec.Setup(x => x.JobSteps).Returns(new Queue(variableSet.Steps.Select(x => x.Object).ToList())); + _ec.Setup(x => x.JobSteps).Returns(new List(variableSet.Steps.Select(x => x.Object).ToList())); // Act. await _stepsRunner.RunAsync(jobContext: _ec.Object); @@ -330,7 +330,7 @@ namespace GitHub.Runner.Common.Tests.Worker { _ec.Object.Result = null; - _ec.Setup(x => x.JobSteps).Returns(new Queue(variableSet.Step.Select(x => x.Object).ToList())); + _ec.Setup(x => x.JobSteps).Returns(new List(variableSet.Step.Select(x => x.Object).ToList())); // Act. await _stepsRunner.RunAsync(jobContext: _ec.Object); @@ -361,7 +361,7 @@ namespace GitHub.Runner.Common.Tests.Worker { _ec.Object.Result = null; - _ec.Setup(x => x.JobSteps).Returns(new Queue(variableSet.Select(x => x.Object).ToList())); + _ec.Setup(x => x.JobSteps).Returns(new List(variableSet.Select(x => x.Object).ToList())); // Act. await _stepsRunner.RunAsync(jobContext: _ec.Object); @@ -391,7 +391,7 @@ namespace GitHub.Runner.Common.Tests.Worker { _ec.Object.Result = null; - _ec.Setup(x => x.JobSteps).Returns(new Queue(variableSet.Select(x => x.Object).ToList())); + _ec.Setup(x => x.JobSteps).Returns(new List(variableSet.Select(x => x.Object).ToList())); // Act. await _stepsRunner.RunAsync(jobContext: _ec.Object); @@ -417,7 +417,7 @@ namespace GitHub.Runner.Common.Tests.Worker _ec.Object.Result = null; - _ec.Setup(x => x.JobSteps).Returns(new Queue(new[] { step1.Object })); + _ec.Setup(x => x.JobSteps).Returns(new List(new[] { step1.Object })); // Act. await _stepsRunner.RunAsync(jobContext: _ec.Object); @@ -455,7 +455,7 @@ namespace GitHub.Runner.Common.Tests.Worker _ec.Object.Result = null; - _ec.Setup(x => x.JobSteps).Returns(new Queue(new[] { step1.Object, step2.Object })); + _ec.Setup(x => x.JobSteps).Returns(new List(new[] { step1.Object, step2.Object })); // Act. await _stepsRunner.RunAsync(jobContext: _ec.Object); @@ -493,7 +493,7 @@ namespace GitHub.Runner.Common.Tests.Worker _ec.Object.Result = null; - _ec.Setup(x => x.JobSteps).Returns(new Queue(new[] { step1.Object, step2.Object })); + _ec.Setup(x => x.JobSteps).Returns(new List(new[] { step1.Object, step2.Object })); // Act. await _stepsRunner.RunAsync(jobContext: _ec.Object); @@ -524,7 +524,7 @@ namespace GitHub.Runner.Common.Tests.Worker _ec.Object.Result = null; - _ec.Setup(x => x.JobSteps).Returns(new Queue(new[] { step1.Object, step2.Object, step3.Object })); + _ec.Setup(x => x.JobSteps).Returns(new List(new[] { step1.Object, step2.Object, step3.Object })); // Act. await _stepsRunner.RunAsync(jobContext: _ec.Object); @@ -560,7 +560,7 @@ namespace GitHub.Runner.Common.Tests.Worker _ec.Object.Result = null; - _ec.Setup(x => x.JobSteps).Returns(new Queue(new[] { step1.Object, step2.Object, step3.Object })); + _ec.Setup(x => x.JobSteps).Returns(new List(new[] { step1.Object, step2.Object, step3.Object })); // Act. await _stepsRunner.RunAsync(jobContext: _ec.Object);