feat: Lint actions #366
feat: Lint actions #366
Conversation
For the moment we kept the old scheme and rules.
It allows existing rules to function as are (they potentially make preparation and post actions in Workflow* Job* functions).
">" stricts newlines with single spaces, but apparently this isn't picked by actionlint/shellcheck
Starting to port rule_id, rule_expression and rule_shellcheck to correctly handle action visits.
Use interface for the Runs block and a normal struct for the Action itself
Select syntax to check based on file name or top-level yaml keys.
|
Marking PR as ready to indicate further work depends on PR review. |
rhysd
force-pushed
the
main
branch
2 times, most recently
from
78e9286
to
0b49da7
Compare
|
What's the status on this? Will this eventually be merged or should I look for another tool to lint action files? I've tried @msw-kialo changes on my action files and it seems to work perfectly. I would like to use this in my actions workflow without having to clone and build their fork. |
|
I am ready to make needed adjustments, rebases, restructure the PR/change etc. |
There was a problem hiding this comment.
Overall, LGTM! 🎉 👏
Left several suggestions.
I also tested this branch locally against ~10 workflows and ~50 actions after I merged in the latest changes (kialo:lint-actions is about 250 commits behind main right now; a few trivial merge-conflicts to resolve).
| // ActionRuns is an interface how the action is executed. Action can be executed as JavaScript, Docker or composite steps. | ||
| type ActionRuns interface { |
There was a problem hiding this comment.
| // ActionRuns is an interface how the action is executed. Action can be executed as JavaScript, Docker or composite steps. | |
| type ActionRuns interface { | |
| // ActionRuns is an interface how the action is executed. Action can be executed as JavaScript, Docker or composite steps. | |
| // https://docs.github.com/en/actions/creating-actions/metadata-syntax-for-github-actions#runs | |
| type ActionRuns interface { |
| @@ -77,6 +78,15 @@ type Command struct { | |||
| Stderr io.Writer | |||
| } | |||
|
|
|||
| func isDir(path string) bool { | |||
| // use a switch to make it a bit cleaner | |||
There was a problem hiding this comment.
| // use a switch to make it a bit cleaner | |
| // TODO: use a switch to make it a bit cleaner |
todo or remove? (leaving suggestions for both; you pick? 😄)
| @@ -77,6 +78,15 @@ type Command struct { | |||
| Stderr io.Writer | |||
| } | |||
|
|
|||
| func isDir(path string) bool { | |||
| // use a switch to make it a bit cleaner | |||
There was a problem hiding this comment.
| // use a switch to make it a bit cleaner |
| var parentDir string | ||
| if l.inputFormat == FileWorkflow { | ||
| parentDir = p.WorkflowsDir() | ||
| } else { | ||
| parentDir = p.RootDir() | ||
| } | ||
| return l.LintDir(parentDir, p) |
There was a problem hiding this comment.
I'm not able to suggest across all needed lines, but can this whole function turn into just:
| var parentDir string | |
| if l.inputFormat == FileWorkflow { | |
| parentDir = p.WorkflowsDir() | |
| } else { | |
| parentDir = p.RootDir() | |
| } | |
| return l.LintDir(parentDir, p) | |
| var parentDir string | |
| if l.inputFormat == FileWorkflow { | |
| parentDir = p.WorkflowsDir() | |
| } else { | |
| parentDir = p.RootDir() | |
| } | |
| return l.LintDirInRepository(parentDir, p) |
(and, delete lines 264-274 above, which looks like duplicate w/ everything in LintDirInRepository)
| } | ||
| if a != nil { | ||
| if err := v.VisitAction(a); err != nil { | ||
| l.debug("error occurred while visiting workflow syntax tree: %v", err) |
There was a problem hiding this comment.
| l.debug("error occurred while visiting workflow syntax tree: %v", err) | |
| l.debug("error occurred while visiting action syntax tree: %v", err) |
| return format | ||
| } | ||
| if strings.Contains(filename, ".github/workflows/") { | ||
| // println("Detect", filename, "as workflow file based (due to its directory)") |
There was a problem hiding this comment.
worth having this as a debug statement?
| return FileWorkflow | ||
| } | ||
| if strings.HasSuffix(filename, "/action.yaml") || strings.HasSuffix(filename, "/action.yml") { | ||
| // println("Detect", filename, "as action filename based (due to its filename)") |
There was a problem hiding this comment.
worth having this as a debug statement?
|
|
||
| // selecting Action if `runs` element is present Workflow otherwise: | ||
| if isNull(node) || len(node.Content) == 0 || node.Content[0].Kind != yaml.MappingNode { | ||
| // println("Defaulted", filename, "as workflow (file is not a yaml mapping)", nodeKindName(node.Kind)) |
There was a problem hiding this comment.
worth having this as a debug statement?
|
|
||
| for i := 0; i < len(node.Content[0].Content); i += 2 { | ||
| if node.Content[0].Content[i].Kind == yaml.ScalarNode && node.Content[0].Content[i].Value == "runs" { | ||
| // println("Detected", filename, "as action workflow (it has a 'runs' key)") |
There was a problem hiding this comment.
worth having this as a debug statement?
| rule.seen = nil | ||
| return nil |
There was a problem hiding this comment.
| rule.seen = nil | |
| return nil | |
| rule.seen = nil | |
| return nil |
I'm not sure if GitHub themselves provides anything, but there is a JSON payload that has the respective values here: #366 Honestly, I don't think these change often enough that it warrants the added complexity in this initial PR. My 2 cents is this can be tracked as a future feature improvement, and implemented if/when it becomes an issue for folks down the road. |
|
Oh @msw-kialo another thing I forgot to comment on - it's worth updating the |
|
@ChrisCarini Thanks for testing it and providing feedback. I am still open to make needed changes to this PR. However, since https://github.com/rhysd/actionlint/releases/tag/v1.7.0:
It appears that @rhysd has other architecture plans. So until I hear where this is going I won't invest more time into this. Actually, I consider just closing this PR as it is apparently not appreciated. |
Hi @msw-kialo - no problem! Thank you for implementing this functionality and posting the PR! 🙏
I understand that's a bit frustrating :( FWIW, I would actually really love it if your fork for this PR was released separately until this repo either (a) decides to provide further comment in pursuit of getting the feature merged in, or (b) decides to implement this support some other way. Ultimately, I really just want this functionality (and personally don't mind who/where it is implemented and released) so I can start using faster. |
This PR tries to address #46 to lint action metadata files (
action.yml/action.yaml) analog to workflow files. In addition to scheme validation this allows finding more sophisticated errors by running pyflake, shellcheck, validation expressions and other cross field references (e.g. outputs). The main advantage is obviously for people writing composite actions.Included changes
This PR tries to cover all essential aspects of this feature and provides example implementation for most of them:
parse*methods to parse action metadata.Implementation decisions
We needed to take a few implementation decisions where it is not clear whether to picked the best option (once they are decided we will squash our changes and mark the PR as ready):
ParseandVisitwere added). At the moment, the visitorspassinterface is extended byVisitActionPreandVisitActionPost, however it reusesvisitStep. This could "crash" (nil value accessed) custom rules asVisitWorkflowPre,VisitJobPremight not be called beforevisitStep: theRuleID,RuleExpressionhad this issue.action_metadata.go) was left unchanged (it appears to be more light-weight and overall simpler to leave as small duplication).LintDirwill only lint arbitrary.yml,.yamlfiles if the directory is.github/workflows. This should be an issue in practice (as workflows needs to be in such directory to be interpreted by GitHub), but it required thetestdata/projectstests to be adapted (specify-input-format=workflowwould also work).actionlintis invoked without extra arguments)action.yml/action.yamlfile within the repository is considered an action metadata file (top level, or in any subdirectory at any level). While it ensures by default any such file is discovered, it might slow if it is a big repository and could cause false positives (the filename isn't that specific). Discovery could be reduced; but, new config option might be needed to handle more complex use-cases.-input-format=workflowwas added to the dog-food CI calls to ensure thetestdataactions aren't discovered.actionlint $dirto lint only files within the specific directory, e.g.actionlint .github/actions(it was originally added for testing purposes but it might be helpful for others, too).inputs.X.default,runs.envs.X,runs.pre-if,runs.post-ifare new. E.g. I suspect special functions likealways(),success()are supported forruns.pre-ifbut I can't proof it.colorandicon) are currently hardcoded. I suspect, a script to extracted them from GitHub's docs should be added?