Defer evaluation of a step's DisplayName until its condition is evalu…

…ated. (actions#2313)

* Defer evaluation of a step's DisplayName until its condition is evaluated.
* Formalize TryUpdateDisplayName and EvaluateDisplayName as members of interface `IStep` (actions#2374)

src/Runner.Worker/ActionRunner.cs

@@ -25,7 +25,6 @@ public enum ActionRunStage
     public interface IActionRunner : IStep, IRunnerService
     {
         ActionRunStage Stage { get; set; }
-        bool TryEvaluateDisplayName(DictionaryContextData contextData, IExecutionContext context);
         Pipelines.ActionStep Action { get; set; }
     }
 
@@ -285,25 +284,67 @@ Action.Reference is Pipelines.RepositoryPathReference repoAction &&
 
         }
 
-        public bool TryEvaluateDisplayName(DictionaryContextData contextData, IExecutionContext context)
+        /// <summary>
+        /// Attempts to update the DisplayName.
+        /// As the "Try..." name implies, this method should never throw an exception.
+        /// Returns true if the DisplayName is already present or it was successfully updated.
+        /// </summary>
+        public bool TryUpdateDisplayName(out bool updated)
+        {
+            updated = false;
+
+            // REVIEW:  This try/catch can be removed if some future implementation of EvaluateDisplayName and UpdateTimelineRecordDisplayName
+            // can make reasonable guarantees that they won't throw an exception.
+            try
+            {
+                // This attempt is only worthwhile at the "Main" stage.
+                //    When the job starts, there's an initial attempt to evaluate the DisplayName.  (see JobExtension::InitializeJob)
+                //    During the "Pre" stage, we expect that no contexts will have changed since the initial evaluation.
+                //    "Main" stage is handled here.
+                //    During the "Post" stage, it no longer matters.
+                if (this.Stage == ActionRunStage.Main && EvaluateDisplayName(this.ExecutionContext.ExpressionValues, this.ExecutionContext, out updated))
+                {
+                    if (updated)
+                    {
+                        this.ExecutionContext.UpdateTimelineRecordDisplayName(this.DisplayName);
+                    }
+                }
+            }
+            catch (Exception ex)
+            {
+                Trace.Warning("Caught exception while attempting to evaulate/update the step's DisplayName.  Exception Details:  {0}", ex);
+            }
+
+            // For consistency with other implementations of TryUpdateDisplayName we use !string.IsNullOrEmpty below,
+            // but note that (at the time of this writing) ActionRunner::DisplayName::get always returns a non-empty string due to its fallback logic.
+            // In other words, the net effect is that this particular implementation of TryUpdateDisplayName will always return true.
+            return !string.IsNullOrEmpty(this.DisplayName);
+        }
+
+
+        /// <summary>
+        /// Attempts to evaluate the DisplayName of this IActionRunner.
+        /// Returns true if the DisplayName is already present or it was successfully evaluated.
+        /// </summary>
+        public bool EvaluateDisplayName(DictionaryContextData contextData, IExecutionContext context, out bool updated)
         {
             ArgUtil.NotNull(context, nameof(context));
             ArgUtil.NotNull(Action, nameof(Action));
 
-            // If we have already expanded the display name, there is no need to expand it again
-            // TODO: Remove the ShouldEvaluateDisplayName check and field post m158 deploy, we should do it by default once the server is updated
+            updated = false;
+            // If we have already expanded the display name, don't bother attempting [re-]expansion.
             if (_didFullyEvaluateDisplayName || !string.IsNullOrEmpty(Action.DisplayName))
             {
-                return false;
+                return true;
             }
 
-            bool didFullyEvaluate;
-            _displayName = GenerateDisplayName(Action, contextData, context, out didFullyEvaluate);
+            _displayName = GenerateDisplayName(Action, contextData, context, out bool didFullyEvaluate);
 
-            // If we evaluated fully mask any secrets
+            // If we evaluated, fully mask any secrets
             if (didFullyEvaluate)
             {
                 _displayName = HostContext.SecretMasker.MaskSecrets(_displayName);
+                updated = true;
             }
             context.Debug($"Set step '{Action.Name}' display name to: '{_displayName}'");
             _didFullyEvaluateDisplayName = didFullyEvaluate;

src/Runner.Worker/JobExtension.cs

@@ -306,13 +306,13 @@ public async Task<List<IStep>> InitializeJob(IExecutionContext jobContext, Pipel
                                 }
                             }
 
-                            actionRunner.TryEvaluateDisplayName(contextData, context);
+                            actionRunner.EvaluateDisplayName(contextData, context, out _);
                             jobSteps.Add(actionRunner);
 
                             if (prepareResult.PreStepTracker.TryGetValue(step.Id, out var preStep))
                             {
                                 Trace.Info($"Adding pre-{action.DisplayName}.");
-                                preStep.TryEvaluateDisplayName(contextData, context);
+                                preStep.EvaluateDisplayName(contextData, context, out _);
                                 preStep.DisplayName = $"Pre {preStep.DisplayName}";
                                 preJobSteps.Add(preStep);
                             }
@@ -328,10 +328,10 @@ public async Task<List<IStep>> InitializeJob(IExecutionContext jobContext, Pipel
                         if (message.ContextData.TryGetValue("inputs", out var pipelineContextData))
                         {
                             var inputs = pipelineContextData.AssertDictionary("inputs");
-                            if (inputs.Any()) 
+                            if (inputs.Any())
                             {
                                 context.Output($"##[group] Inputs");
-                                foreach (var input in inputs) 
+                                foreach (var input in inputs)
                                 {
                                     context.Output($"  {input.Key}: {input.Value}");
                                 }

src/Runner.Worker/JobExtensionRunner.cs

@@ -1,6 +1,7 @@
 using System;
 using System.Threading.Tasks;
 using GitHub.DistributedTask.ObjectTemplating.Tokens;
+using GitHub.DistributedTask.Pipelines.ContextData;
 
 namespace GitHub.Runner.Worker
 {
@@ -32,5 +33,18 @@ public async Task RunAsync()
         {
             await _runAsync(ExecutionContext, _data);
         }
+
+        public bool TryUpdateDisplayName(out bool updated)
+        {
+            updated = false;
+            return !string.IsNullOrEmpty(this.DisplayName);
+        }
+
+        public bool EvaluateDisplayName(DictionaryContextData contextData, IExecutionContext context, out bool updated)
+        {
+            updated = false;
+            return !string.IsNullOrEmpty(this.DisplayName);
+        }
+
     }
 }

src/Runner.Worker/StepsRunner.cs

@@ -1,21 +1,16 @@
 using System;
 using System.Collections.Generic;
-using System.Linq;
-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
 {
@@ -26,6 +21,8 @@ public interface IStep
         string DisplayName { get; set; }
         IExecutionContext ExecutionContext { get; set; }
         TemplateToken Timeout { get; }
+        bool TryUpdateDisplayName(out bool updated);
+        bool EvaluateDisplayName(DictionaryContextData contextData, IExecutionContext context, out bool updated);
         Task RunAsync();
     }
 
@@ -195,6 +192,12 @@ public async Task RunAsync(IExecutionContext jobContext)
                         }
                         else
                         {
+                            // This is our last, best chance to expand the display name.  (At this point, all the requirements for successful expansion should be met.)
+                            // That being said, evaluating the display name should still be considered as a "best effort" exercise.  (It's not critical or paramount.)
+                            // For that reason, we call a safe "Try..." wrapper method to ensure that any potential problems we encounter in evaluating the display name
+                            // don't interfere with our ultimate goal within this code block:  evaluation of the condition.
+                            step.TryUpdateDisplayName(out _);
+
                             try
                             {
                                 var templateEvaluator = step.ExecutionContext.ToPipelineTemplateEvaluator(conditionTraceWriter);
@@ -256,14 +259,6 @@ public async Task RunAsync(IExecutionContext jobContext)
 
         private async Task RunStepAsync(IStep step, CancellationToken jobCancellationToken)
         {
-            // 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);
-            }
-
             // Start the step
             Trace.Info("Starting the step.");
             step.ExecutionContext.Debug($"Starting: {step.DisplayName}");

src/Test/L0/Worker/ActionRunnerL0.cs

@@ -3,20 +3,14 @@
 using GitHub.DistributedTask.Pipelines;
 using GitHub.DistributedTask.Pipelines.ContextData;
 using GitHub.DistributedTask.WebApi;
-using GitHub.Runner.Common.Util;
 using GitHub.Runner.Worker;
-using GitHub.Runner.Worker.Container;
 using GitHub.Runner.Worker.Handlers;
 using Moq;
 using Newtonsoft.Json.Linq;
 using System;
 using System.Collections.Generic;
-using System.IO;
-using System.IO.Compression;
-using System.Reflection;
 using System.Runtime.CompilerServices;
 using System.Threading;
-using System.Threading.Tasks;
 using Xunit;
 using Pipelines = GitHub.DistributedTask.Pipelines;
 
@@ -149,11 +143,12 @@ public void EvaluateLegacyDisplayName()
             _context.Add("matrix", matrixData);
 
             // Act
-            // Should not do anything if we don't have a displayNameToken to expand
-            var didUpdateDisplayName = _actionRunner.TryEvaluateDisplayName(_context, _actionRunner.ExecutionContext);
+            // Should report success with no updated required if there's already a valid display name.
+            var validDisplayName = _actionRunner.EvaluateDisplayName(_context, _actionRunner.ExecutionContext, out bool updated);
 
             // Assert
-            Assert.False(didUpdateDisplayName);
+            Assert.True(validDisplayName);
+            Assert.False(updated);
             Assert.Equal(actionDisplayName, _actionRunner.DisplayName);
         }
 
@@ -183,13 +178,51 @@ public void EvaluateExpansionOfDisplayNameToken()
 
             // Act
             // Should expand the displaynameToken and set the display name to that
-            var didUpdateDisplayName = _actionRunner.TryEvaluateDisplayName(_context, _actionRunner.ExecutionContext);
+            var validDisplayName = _actionRunner.EvaluateDisplayName(_context, _actionRunner.ExecutionContext, out bool updated);
 
             // Assert
-            Assert.True(didUpdateDisplayName);
+            Assert.True(validDisplayName);
+            Assert.True(updated);
             Assert.Equal(expectedString, _actionRunner.DisplayName);
         }
 
+        [Fact]
+        [Trait("Level", "L0")]
+        [Trait("Category", "Worker")]
+        public void IgnoreDisplayNameTokenWhenDisplayNameIsExplicitlySet()
+        {
+            var explicitDisplayName = "Explcitly Set Name";
+
+            // Arrange
+            Setup();
+            var actionId = Guid.NewGuid();
+            var action = new Pipelines.ActionStep()
+            {
+                Name = "action",
+                Id = actionId,
+                DisplayName = explicitDisplayName,
+                DisplayNameToken = new BasicExpressionToken(null, null, null, "matrix.node"),
+            };
+
+            _actionRunner.Action = action;
+
+            var matrixData = new DictionaryContextData
+            {
+                ["node"] = new StringContextData("8")
+            };
+            _context.Add("matrix", matrixData);
+
+            // Act
+            // Should ignore the displayNameToken since there's already an explicit value for DisplayName
+            var validDisplayName = _actionRunner.EvaluateDisplayName(_context, _actionRunner.ExecutionContext, out bool updated);
+
+            // Assert
+            Assert.True(validDisplayName);
+            Assert.False(updated);
+            Assert.Equal(explicitDisplayName, _actionRunner.DisplayName);
+        }
+
+
         [Fact]
         [Trait("Level", "L0")]
         [Trait("Category", "Worker")]
@@ -218,10 +251,11 @@ public void EvaluateExpansionOfScriptDisplayName()
 
             // Act
             // Should expand the displaynameToken and set the display name to that
-            var didUpdateDisplayName = _actionRunner.TryEvaluateDisplayName(_context, _actionRunner.ExecutionContext);
+            var validDisplayName = _actionRunner.EvaluateDisplayName(_context, _actionRunner.ExecutionContext, out bool updated);
 
             // Assert
-            Assert.True(didUpdateDisplayName);
+            Assert.True(validDisplayName);
+            Assert.True(updated);
             Assert.Equal("Run 8", _actionRunner.DisplayName);
         }
 
@@ -246,10 +280,11 @@ public void EvaluateExpansionOfContainerDisplayName()
 
             // Act
             // Should expand the displaynameToken and set the display name to that
-            var didUpdateDisplayName = _actionRunner.TryEvaluateDisplayName(_context, _actionRunner.ExecutionContext);
+            var validDisplayName = _actionRunner.EvaluateDisplayName(_context, _actionRunner.ExecutionContext, out bool updated);
 
             // Assert
-            Assert.True(didUpdateDisplayName);
+            Assert.True(validDisplayName);
+            Assert.True(updated);
             Assert.Equal("Run TestImageName:latest", _actionRunner.DisplayName);
         }
 
@@ -272,10 +307,11 @@ public void EvaluateDisplayNameWithoutContext()
 
             // Act
             // Should not do anything if we don't have context on the display name
-            var didUpdateDisplayName = _actionRunner.TryEvaluateDisplayName(_context, _actionRunner.ExecutionContext);
+            var validDisplayName = _actionRunner.EvaluateDisplayName(_context, _actionRunner.ExecutionContext, out bool updated);
 
             // Assert
-            Assert.False(didUpdateDisplayName);
+            Assert.False(validDisplayName);
+            Assert.False(updated);
             // Should use the pretty display name until we can eval
             Assert.Equal("${{ matrix.node }}", _actionRunner.DisplayName);
         }