Composite Actions: Support Env Flow #557

ethanchewy · 2020-06-18T20:03:03Z

This is branched off of my previous PR #549

In this PR, I build in support for environment variables for a Composite Action and its steps as described in the ADR: https://github.com/actions/runner/blob/users/ethanchewy/compositeADR/docs/adrs/0549-composite-run-steps.md#environment

action.yml

runs:
    using: "composite"
    steps: 
      - run: echo Composite Action Server ${{ env.SERVER }}
      - run: echo Server changed to ${{ env.SERVER }}

workflow.yml

jobs:
  build:
    runs-on: self-hosted
    steps:
    - uses: actions/checkout@v2
    - id: composite1
      uses: ethanchewy/test-composite@env
      with:
        your_name: "Octocat"
      env:
        NAME3: testasdfasdfads
    - name: test outputs
      run: echo Server: ${{ env.SERVER }}

Demo: https://github.com/ethanchewy/testing-actions/runs/846467581?check_suite_focus=true

This is an experimental feature that can only be turned on via turning the feature flag on:
export TESTING_COMPOSITE_ACTIONS_ALPHA=1
./run.sh

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

…actions/runner into users/ethanchewy/compositeenv

ethanchewy · 2020-06-18T21:37:01Z

src/Runner.Worker/action_yaml.json

@@ -88,6 +88,7 @@
            "mapping": {
                "properties": {
                    "using": "non-empty-string",
+                    "env": "runs-env",


Since, container-runs-env's structure is used by both composite actions and container actions, I decided to change the name so that both types of runs can use it.

Moreover, to support this change, I made sure to look at all files that contained container-runs-env and replace it with `run-env'

…actions/runner into users/ethanchewy/compositeenv

ethanchewy · 2020-06-22T14:02:30Z

~~TODO: We need to add context so that env variables are evaluated when Runner jobs are printed (ex: Run echo Name3 changed to $NAME3 should instead be Run echo Name3 changed to fdasdfadsf~~

Nevermind, in both the composite action and workflow steps, the environment contexts are evaluated correctly: https://github.com/ethanchewy/testing-actions/runs/795652488?check_suite_focus=true

ethanchewy · 2020-06-22T14:24:01Z

Ready for review!

…actions/runner into users/ethanchewy/compositeenv

ethanchewy · 2020-06-30T20:01:15Z

TODO: Remove only top level env token support.

I need to remove most of my changes from the PR

ericsciple · 2020-06-30T20:07:57Z

src/Runner.Worker/ActionManager.cs

@@ -1285,6 +1287,7 @@ public sealed class CompositeActionExecutionData : ActionExecutionData
        public override bool HasPre => false;
        public override bool HasPost => false;
        public List<Pipelines.ActionStep> Steps { get; set; }
+        public MappingToken Environment { get; set; }


ericsciple · 2020-06-30T20:08:12Z

src/Runner.Worker/ActionManager.cs

@@ -400,6 +400,8 @@ public Definition LoadAction(IExecutionContext executionContext, Pipelines.Actio
                        var compositeAction = definition.Data.Execution as CompositeActionExecutionData;
                        Trace.Info($"Load {compositeAction.Steps.Count} action steps.");
                        Trace.Verbose($"Details: {StringUtil.ConvertToJson(compositeAction.Steps)}");
+                        Trace.Info($"Load: {compositeAction.Environment} environment steps");


remove these 2 lines

ericsciple · 2020-06-30T20:08:22Z

src/Runner.Worker/ActionManifestManager.cs

@@ -27,6 +27,7 @@ public interface IActionManifestManager : IRunnerService

        Dictionary<string, string> EvaluateContainerEnvironment(IExecutionContext executionContext, MappingToken token, IDictionary<string, PipelineContextData> extraExpressionValues);

+        public Dictionary<string, string> EvaluateCompositeActionEnvironment(IExecutionContext executionContext, MappingToken token, IDictionary<string, PipelineContextData> extraExpressionValues);


ericsciple · 2020-06-30T20:08:33Z

src/Runner.Worker/ActionManifestManager.cs

@@ -74,6 +75,8 @@ public ActionDefinitionData Load(IExecutionContext executionContext, string mani
                }

                var actionMapping = token.AssertMapping("action manifest root");
+                var envComposite = default(MappingToken);


ethanchewy · 2020-06-30T20:19:11Z

src/Runner.Worker/Handlers/CompositeActionHandler.cs

+                envData[env.Key] = env.Value;
+            }
+            // Overwrite with current env
+            foreach (var env in compositeEnvData)


Remove for now since we don't need to supprot outputs in action

ethanchewy · 2020-06-30T20:21:19Z

src/Runner.Worker/Handlers/CompositeActionHandler.cs

+            // Add the composite action environment variables to each step.
+            // If the key already exists, we override it since the composite action env variables will have higher precedence
+            // Note that for each composite action step, it's environment variables will be set in the StepRunner automatically
+            var compositeEnvData = manifestManager.EvaluateCompositeActionEnvironment(ExecutionContext, Data.Environment, extraExpressionValues);


Remove Data.Environment

ethanchewy · 2020-07-07T16:38:46Z

Removed all of my code that supported the env token in the composite action.

Updated Description above to reflect this.

ericsciple · 2020-07-07T20:34:18Z

lets make sure to test this:

steps:
  - uses: myorg/mycompositeaction@v1
     env: 
      foo: this is foo
  - run: printenv|sort        # prints baz


# my composite action
steps:
  - env: 
      bar: this is bar 
    run: | 
      echo '::set-env name=baz::this is baz'
      printenv|sort        # prints foo and bar

ericsciple · 2020-07-07T21:12:47Z

let's go with this behavior

steps:
  - run: echo ::set-env name=foo::this is foo 1
  - uses: myorg/mycompositeaction@v1
     env: 
      foo: this is foo 2

# my composite action
steps:
  - printenv|sort           # should get foo 2 (more specific)
  - run: echo ::set-env name=foo::this is foo 3
  - printenv|sort           # will get foo 2 also... let's mark it as a bug and figure out how to solve w/ nested step runner

ethanchewy · 2020-07-07T22:16:00Z

First test works as intended:
https://github.com/ethanchewy/testing-actions/runs/847543901?check_suite_focus=true

Second test works as intended as well:
https://github.com/ethanchewy/testing-actions/runs/847558293?check_suite_focus=true

As discussed with Ting and Eric, we will most likely have to refactor this in the future. See above comments by Eric for reference.

Pushing changes soon.

ericsciple

lgtm

we'll figure out how to simplify w/ nested composite steps runner (when we solve timeout)

…tions/runner into users/ethanchewy/compositeenv

* 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 * Sort usings in Composite Action Handler * Change 0 to location * Update context variables in composite action yaml * Add helpful error message for null steps * 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 * Figure out how to handle set-env edge cases * formatting * fix unit tests * Fix windows unit test syntax error

ethanchewy added 3 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

ethanchewy changed the base branch from master to users/ethanchewy/compositetest2 June 18, 2020 20:05

ethanchewy added 4 commits June 18, 2020 16:13

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

ethanchewy commented Jun 18, 2020

View reviewed changes

ethanchewy added 9 commits June 19, 2020 11:22

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

ethanchewy mentioned this pull request Jun 22, 2020

Composite Actions Support for Multiple Run Steps #549

Merged

Remove comment

37849dc

ethanchewy requested review from TingluoHuang and ericsciple June 22, 2020 14:24

ethanchewy added 2 commits June 22, 2020 11:31

Remove TODO message for context

94e7b47

Add verbose trace logs which are only viewable by devs

a254442

ethanchewy mentioned this pull request Jun 22, 2020

Composite Run Steps ADR #554

Merged

ethanchewy added 3 commits June 22, 2020 12:56

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

fbef557

…actions/runner into users/ethanchewy/compositeenv

Sort usings in Composite Action Handler

0d5e84b

Change 0 to location

9ec7047

Fix Workflow Step Env overiding Parent Env

57d59fc

ericsciple reviewed Jun 30, 2020

View reviewed changes

ethanchewy commented Jun 30, 2020

View reviewed changes

Remove env in composite action scope

b63f987

ethanchewy added 5 commits July 7, 2020 12:40

Clean up

da1e2b0

Revert back

5d61145

revert back

4ccac8c

add back envToken

0041023

Remove unnecessary code

4b3ec9f

ethanchewy requested a review from ericsciple July 7, 2020 19:25

ethanchewy added 2 commits July 7, 2020 18:16

Figure out how to handle set-env edge cases

96bff6a

formatting

11b9cac

ericsciple approved these changes Jul 7, 2020

View reviewed changes

ethanchewy changed the base branch from users/ethanchewy/compositetest2 to master July 7, 2020 22:45

ethanchewy and others added 4 commits July 7, 2020 18:47

Merge branch 'master' into users/ethanchewy/compositeenv

b712ba2

fix unit tests

a58ac2e

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

5178468

…tions/runner into users/ethanchewy/compositeenv

Fix windows unit test syntax error

881e8e7

ethanchewy merged commit d42c9da into master Jul 8, 2020

ethanchewy mentioned this pull request Aug 7, 2020

Next Steps for Fully Functioning Composite Actions #646

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Composite Actions: Support Env Flow #557

Composite Actions: Support Env Flow #557

ethanchewy commented Jun 18, 2020 •

edited

Loading

ethanchewy Jun 18, 2020

ethanchewy commented Jun 22, 2020 •

edited

Loading

ethanchewy commented Jun 22, 2020

ethanchewy commented Jun 30, 2020 •

edited

Loading

ericsciple Jun 30, 2020

ericsciple Jun 30, 2020

ericsciple Jun 30, 2020

ericsciple Jun 30, 2020

ethanchewy Jun 30, 2020

ethanchewy Jun 30, 2020

ethanchewy commented Jul 7, 2020

ericsciple commented Jul 7, 2020 •

edited by ethanchewy

Loading

ericsciple commented Jul 7, 2020

ethanchewy commented Jul 7, 2020

ericsciple left a comment

		@@ -27,6 +27,7 @@ public interface IActionManifestManager : IRunnerService

		Dictionary<string, string> EvaluateContainerEnvironment(IExecutionContext executionContext, MappingToken token, IDictionary<string, PipelineContextData> extraExpressionValues);

		public Dictionary<string, string> EvaluateCompositeActionEnvironment(IExecutionContext executionContext, MappingToken token, IDictionary<string, PipelineContextData> extraExpressionValues);

Composite Actions: Support Env Flow #557

Composite Actions: Support Env Flow #557

Conversation

ethanchewy commented Jun 18, 2020 • edited Loading

ethanchewy Jun 18, 2020

Choose a reason for hiding this comment

ethanchewy commented Jun 22, 2020 • edited Loading

ethanchewy commented Jun 22, 2020

ethanchewy commented Jun 30, 2020 • edited Loading

ericsciple Jun 30, 2020

Choose a reason for hiding this comment

ericsciple Jun 30, 2020

Choose a reason for hiding this comment

ericsciple Jun 30, 2020

Choose a reason for hiding this comment

ericsciple Jun 30, 2020

Choose a reason for hiding this comment

ethanchewy Jun 30, 2020

Choose a reason for hiding this comment

ethanchewy Jun 30, 2020

Choose a reason for hiding this comment

ethanchewy commented Jul 7, 2020

ericsciple commented Jul 7, 2020 • edited by ethanchewy Loading

ericsciple commented Jul 7, 2020

ethanchewy commented Jul 7, 2020

ericsciple left a comment

Choose a reason for hiding this comment

ethanchewy commented Jun 18, 2020 •

edited

Loading

ethanchewy commented Jun 22, 2020 •

edited

Loading

ethanchewy commented Jun 30, 2020 •

edited

Loading

ericsciple commented Jul 7, 2020 •

edited by ethanchewy

Loading