Make inputs consistent between action and runner #211
Conversation
robertbrignull
force-pushed
the
robertbrignull/consistent_inputs
branch
from
a5748ab to
c1cee53
Compare
marcogario
left a comment
marcogario
left a comment
There was a problem hiding this comment.
LGTM!
The CodeQL queries are 💖 , but I would suggest you get someone with more experience to review them.
| if (value === undefined || value.length === 0) { | ||
| throw new Error(`${paramName} environment variable must be set`); | ||
| } | ||
| core.debug(`${paramName}=${value}`); |
There was a problem hiding this comment.
I think this is a false positive.
There was a problem hiding this comment.
I agree I think this is. It's happening because in getCodeQLActionRepository we abort early when not in actions mode and the query doesn't understand this. I will fix that but perhaps would be best in a separate PR. I'll open an issue for it as I don't think there is one now. For this PR I think it's enough to see there's 1 new and 1 fixed alert of this type.
|
I was wondering why this wasn't opening alerts for the uses of |
|
The alert changes now look correct. One new and one fixed alert because we've moved some code to a different file. |
joshhale
left a comment
joshhale
left a comment
There was a problem hiding this comment.
I'm not familiar with the javascript QL library, but the queries lgtm.
| exists(YAMLMapping value | | ||
| value = getRootNode().(YAMLMapping).lookup("inputs").(YAMLMapping).lookup(input) and | ||
| (exists(value.lookup("default")) or | ||
| value.lookup("required").(YAMLBool).getBoolValue() = true)) |
There was a problem hiding this comment.
Yes unfortunately. This is not a predicate that exists or doesn't, but rather it "returns" a true or false boolean value. In CodeQL I think you need to explicitly use = true.
| } | ||
|
|
||
| /** | ||
| * The function that is the entrypoint to this action. |
There was a problem hiding this comment.
Technically, I guess it's the typescript source for the function that is the entrypoint to the action.
...not sure if it's important to make that distinction explicit though.
There was a problem hiding this comment.
I'm not sure either. I think I'll just leave it. I think it's implicit that these queries are on the typescript source code instead of the compiled javascript, assuming that's what you meant as the alternative source for the function.
This is partially an alternative PR to #208 but I thought I'd open it anyway. This PR also goes further by:
actions-util.tsthat allows us to place higher standards onutil.tsto work on actions and the runner and makes it easier to see the division of what code runs where.core.inputand point to the correct custom function.Merge / deployment checklist