-
Notifications
You must be signed in to change notification settings - Fork 150
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
feat: Lint actions #366
base: main
Are you sure you want to change the base?
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. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
worth having this as a debug statement?
rule.seen = nil | ||
return nil |
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.
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):
Parse
andVisit
were added). At the moment, the visitorspass
interface is extended byVisitActionPre
andVisitActionPost
, however it reusesvisitStep
. This could "crash" (nil value accessed) custom rules asVisitWorkflowPre
,VisitJobPre
might not be called beforevisitStep
: theRuleID
,RuleExpression
had this issue.action_metadata.go
) was left unchanged (it appears to be more light-weight and overall simpler to leave as small duplication).LintDir
will only lint arbitrary.yml
,.yaml
files 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/projects
tests to be adapted (specify-input-format=workflow
would also work).actionlint
is invoked without extra arguments)action.yml
/action.yaml
file 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=workflow
was added to the dog-food CI calls to ensure thetestdata
actions aren't discovered.actionlint $dir
to 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-if
are new. E.g. I suspect special functions likealways()
,success()
are supported forruns.pre-if
but I can't proof it.color
andicon
) are currently hardcoded. I suspect, a script to extracted them from GitHub's docs should be added?