Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 Actions Support for Multiple Run Steps #549
Composite Actions Support for Multiple Run Steps #549
Changes from 2 commits
038e5e2
180a687
5843362
941a24e
9939cf5
5988076
00a736d
e4dfd0e
a254442
0d5e84b
9ec7047
8aadbbd
f9b28c7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will print
Action steps: Object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the json representation is desired (too noisy? esp with large inline scripts?) then see StringUtil or IOUtil (has function to convert to/from json)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TingluoHuang thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also,
compositeAction.Steps
is of typeList<Pipelines.ActionStep>
. If we were to output this, what would be the best way to output the values in a json string format?Right now, this Trace.info statement returns the object string representation:
GitHub.DistributedTask.Pipelines.ActionStep
If we were to print out the steps, it might make more sense to print them out in the
LoadCompositeSteps
functionThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From @TingluoHuang (thanks Ting!!) :
So, I'll do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this error give the user enough context to know where the problem is? Like does it convey that an error happened when loading the path-to-action.yml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess we would have the same problem with the other place too (in this file)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this error makes sense and is appropriate.
At the top of
ConvertRuns
:var stepsLoaded = default(List<Pipelines.ActionStep>);
Then, we check in the
action.yml
file to see if there is anysteps
token:If there are any issues with loading the steps, there would be an error reported in the
LoadCompositeSteps()
orLoadStep()
function in the pipeline template evaulator. If the steps attribute is empty, there would be no error and LoadCompositeSteps would return the default value of List which is equivalent to null.Thus, if
stepsLoaded
is null, it must be because there was nosteps
attribute written in theaction.yml
by the user.Thus, I believe throwing this message is appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Add a more helpful error message + including file name, etc. to show user that it's because of their yaml file
(Just add TODO for now)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm worried a little bit because we're not adding the contexts from the job message, like we normally do when creating steps.
I know this is what Post does, but wouldnt surprise me if there is a bug there too.
It may be safe, since we're copying the root node (maybe job message contexts copied there first so safe?)
Let's add a TODO comment in the code like "// TODO: confirm whether not copying message contexts is safe".
I can help look into this more. It's tedious because values are delay expanded sometimes (sometimes evaluated early, like DisplayName). I wish the code were designed simpler... hmm thinking about alternatives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added TODO. Leaving open for visibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After more testing for the env flow, I realized that the display name does not evaluate the env variables. This has to do with not passing the context. See: #557 (comment)Ex: https://github.com/ethanchewy/testing-actions/runs/795652488?check_suite_focus=true
Building in support will occur in the env flow PR: #557
Never mind, in both the composite action and workflow steps, the environment contexts are evaluated correctly: https://github.com/ethanchewy/testing-actions/actions/runs/143774560
Still investigating but for now I think the contexts are handled correctly for the composite action steps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tl;dr The context is exposed correctly in env flow PR and solves this concern
In the env flow PR, I handle the contexts for each correctly: https://github.com/actions/runner/pull/557/files#diff-e3d54b1d0cbdce4d358272df29857cf2R225 by setting the
step.EnvironmentVariables
to the environment variables.Demo: https://github.com/ethanchewy/testing-actions/actions/runs/143774560
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, since we add each of our composite steps to JobSteps, eventually each of the steps will be run via the ScriptHandler.
In the ScriptHandler, it exposes the context to the step/job: https://github.com/actions/runner/blob/master/src/Runner.Worker/Handlers/ScriptHandler.cs#L248-L258
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sort
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
system first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
named at the bottom