Skip to content
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

Implement env context, hashfiles, expression for input's default and post-if for wrapper action #120

Merged
merged 1 commit into from
Oct 5, 2019

Conversation

TingluoHuang
Copy link
Member

@TingluoHuang TingluoHuang commented Sep 28, 2019

hashFiles() expression function:
https://github.com/github/pe-actions-runtime/issues/85

env context for workflow variables:
https://github.com/github/pe-actions-runtime/issues/86

allow expression for input's default value in action.yml:
https://github.com/github/pe-actions-runtime/issues/112

allow wrapper action definition post-if:
https://github.com/github/pe-actions-runtime/issues/113

@TingluoHuang TingluoHuang reopened this Sep 28, 2019
@TingluoHuang TingluoHuang reopened this Sep 28, 2019
@TingluoHuang TingluoHuang force-pushed the users/tihuang/envcontext branch 2 times, most recently from 36e1236 to b1009c0 Compare September 29, 2019 00:11
@TingluoHuang TingluoHuang changed the title prototype env context and hashfiles Imolement env context and hashfiles Oct 2, 2019
@TingluoHuang TingluoHuang changed the title Imolement env context and hashfiles Implement env context and hashfiles Oct 2, 2019
@bryanmacfarlane
Copy link
Member

Clarifying that this isn't prototyping. This is implementing and it's time critical for GA. This needs land with production level quality code.

@bryanmacfarlane
Copy link
Member

Is this ready for review? If so, we might not be able to depend on eric and find another reviewer. Eric is out.

@TingluoHuang
Copy link
Member Author

@TingluoHuang TingluoHuang changed the title Implement env context and hashfiles Implement env context, hashfiles, expression for input's default and post-if for wrapper action Oct 4, 2019
@@ -83,6 +85,18 @@
],
"string": {}
},
"input-default-context": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May want to consider the name "string-default-context"

envContext[name] = new StringContextData(value);
#else
var envContext = ExpressionValues["env"] as CaseSensitiveDictionaryContextData;
envContext[name] = new StringContextData(value);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super nit: can you pull out the envContext[name] line so we don't have to duplicate it.

@@ -22,7 +22,7 @@
"input": {
"mapping": {
"properties": {
"default": "string"
"default": "input-default-context"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we support bool and numbers as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with is string in the workflow schema, so the default should be string as well.

public abstract ActionExecutionType ExecutionType { get; }

public abstract bool HasCleanup { get; }

public string CleanupCondition
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super nit:
I think you can do private string CleanupCondition{ get; set; } = $"{Constants.Expressions.Always}()";

@TingluoHuang TingluoHuang force-pushed the users/tihuang/envcontext branch 2 times, most recently from 023b4ce to c7123c7 Compare October 5, 2019 00:39
@TingluoHuang TingluoHuang merged commit 0dca481 into master Oct 5, 2019
@TingluoHuang TingluoHuang deleted the users/tihuang/envcontext branch October 5, 2019 02:52
volker-raschek pushed a commit to dedalus-cis4u/github-runner-role that referenced this pull request Jul 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants