-
Notifications
You must be signed in to change notification settings - Fork 877
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
Template evaluation in action.yml #56
Conversation
f425c9c
to
c2fafc0
Compare
c2fafc0
to
b31f9c8
Compare
[ServiceLocator(Default = typeof(ActionManifestManager))] | ||
public interface IActionManifestManager : IRunnerService | ||
{ | ||
ActionDefinitionData LoadManifestFile(IExecutionContext executionContext, string manifestFile); |
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.
Shoudl this just be LoadManifest (i.e. is "File" redundant?)
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.
Manifest is also redundant since it's the manifestmanager. Just load?
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.
sure.
|
||
List<string> EvaluateContainerArguments(IExecutionContext executionContext, SequenceToken token, IDictionary<String, PipelineContextData> contextData); | ||
|
||
Dictionary<String, String> EvaluateContainerEnvironments(IExecutionContext executionContext, MappingToken token, IDictionary<String, PipelineContextData> contextData); |
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.
Environment (singular)?
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.
fixed
base.Initialize(hostContext); | ||
|
||
var assembly = Assembly.GetExecutingAssembly(); | ||
var json = default(String); |
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.
string
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.
fixed
return actionDefinition; | ||
} | ||
|
||
public List<string> EvaluateContainerArguments( |
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.
nice
}; | ||
} | ||
} | ||
else if (string.Equals(usingToken.Value, "node", StringComparison.OrdinalIgnoreCase) || |
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.
can we reduce this to just node12
?
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.
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.
All the actions currently use node12
so we should be good.
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.
cool, will remove "node"
/// <summary> | ||
/// Converts a YAML file into a TemplateToken | ||
/// </summary> | ||
internal sealed class YamlObjectReader : IObjectReader |
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 the fork, lets move everything into webapi and kill pipelines.yml. This should come from the SDK
src/Runner.Worker/ActionRunner.cs
Outdated
@@ -11,6 +11,11 @@ | |||
using Pipelines = GitHub.DistributedTask.Pipelines; | |||
using GitHub.Runner.Common; | |||
using GitHub.Runner.Sdk; | |||
using GitHub.DistributedTask.ObjectTemplating.Schema; |
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.
fixed
@@ -89,6 +113,18 @@ public async Task RunAsync() | |||
container.ContainerEntryPointArgs = Inputs.GetValueOrDefault("args"); | |||
} | |||
|
|||
if (Data.Environment != null) |
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.
will this unintentionally clobber action-author-defined env?
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.
what's the guidance? namespace to avoid collision?
src/Runner.Worker/action_yaml.json
Outdated
"mapping": { | ||
"properties": { | ||
"description": "string", | ||
"required": "boolean", |
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.
should we validate required? otherwise should we remove these (should validation fail if we hit keys here we dont understand?)
Can you update the description of the PR to give everyone context? |
Hooks allowed to work even when context isn't returned
add arm64 support
Creating or uploading artifacts needs a valid accesstoken, downloading without it is still allowed
Leverage schema driven yaml parser to read the action.yml file, so we can parse out all the
${{expression}}
expressions inside the yaml file and run through template evaluation with them.ex: