feat: Lint actions #366

msw-kialo · 2023-10-17T13:36:47Z

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:

Add new parse* methods to parse action metadata.
Add a new input-format parameter (both to the CLI and linteropts) to decide whether the input should be treated as workflow file or action metadata file.
Adapt rules if needed to handle action metadata files, too.
Add new branding rule to validate branding values.
Adapt the playground to allow linting of actions, too.
Add tests to correctly valid common actions and caught various issues.

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):

API stability: the public interface was kept unchanged unchanged (if needed function alias like Parse and Visit were added). At the moment, the visitors pass interface is extended by VisitActionPre and VisitActionPost, however it reuses visitStep. This could "crash" (nil value accessed) custom rules as VisitWorkflowPre, VisitJobPre might not be called before visitStep: the RuleID, RuleExpression had this issue.
parseAction vs ActionMetadata: The local action metadata cache (action_metadata.go) was left unchanged (it appears to be more light-weight and overall simpler to leave as small duplication).
LintDir behavior: to keep the change simple, 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 the testdata/projects tests to be adapted (specify -input-format=workflow would also work).
Default behavior: actions are linted by default (at least if actionlint is invoked without extra arguments)
Action location: At the moment, any 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 the testdata actions aren't discovered.
CLI shortcut: this PR added 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).
Expression values: Sadly, we haven't found anything in the GitHub documentation which expressions are supported action metadata files (probably the step files are the same, however, inputs.X.default, runs.envs.X, runs.pre-if, runs.post-if are new. E.g. I suspect special functions like always(), success() are supported for runs.pre-if but I can't proof it.
Branding values: the allowed branding values (color and icon) are currently hardcoded. I suspect, a script to extracted them from GitHub's docs should be added?

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.

msw-kialo · 2023-10-30T14:02:34Z

Marking PR as ready to indicate further work depends on PR review.

Ezri-Mudde · 2024-03-12T12:33:49Z

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.

msw-kialo · 2024-03-21T13:46:03Z

I am ready to make needed adjustments, rebases, restructure the PR/change etc.
However, I am waiting for a first review from @rhysd before I invest more time in a potentially wrong direction.

ChrisCarini

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).

ChrisCarini · 2024-06-26T11:09:11Z

ast.go

+// ActionRuns is an interface how the action is executed. Action can be executed as JavaScript, Docker or composite steps.
+type ActionRuns interface {


Suggested change

// 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 {

ChrisCarini · 2024-06-26T11:11:01Z

command.go

@@ -77,6 +78,15 @@ type Command struct {
 	Stderr io.Writer
 }

+func isDir(path string) bool {
+	// use a switch to make it a bit cleaner


Suggested change

// 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? 😄)

ChrisCarini · 2024-06-26T11:11:08Z

command.go

@@ -77,6 +78,15 @@ type Command struct {
 	Stderr io.Writer
 }

+func isDir(path string) bool {
+	// use a switch to make it a bit cleaner


Suggested change

// use a switch to make it a bit cleaner

ChrisCarini · 2024-06-26T11:22:14Z

linter.go

+	var parentDir string
+	if l.inputFormat == FileWorkflow {
+		parentDir = p.WorkflowsDir()
+	} else {
+		parentDir = p.RootDir()
+	}
+	return l.LintDir(parentDir, p)


I'm not able to suggest across all needed lines, but can this whole function turn into just:

Suggested change

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)

ChrisCarini · 2024-06-26T11:26:35Z

linter.go

+		}
+		if a != nil {
+			if err := v.VisitAction(a); err != nil {
+				l.debug("error occurred while visiting workflow syntax tree: %v", err)


Suggested change

l.debug("error occurred while visiting workflow syntax tree: %v", err)

l.debug("error occurred while visiting action syntax tree: %v", err)

ChrisCarini · 2024-06-26T11:49:16Z

parse.go

+		return format
+	}
+	if strings.Contains(filename, ".github/workflows/") {
+		// println("Detect", filename, "as workflow file based (due to its directory)")


worth having this as a debug statement?

ChrisCarini · 2024-06-26T11:49:20Z

parse.go

+		return FileWorkflow
+	}
+	if strings.HasSuffix(filename, "/action.yaml") || strings.HasSuffix(filename, "/action.yml") {
+		// println("Detect", filename, "as action filename based (due to its filename)")


worth having this as a debug statement?

ChrisCarini · 2024-06-26T11:49:33Z

parse.go

+
+	// 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))


worth having this as a debug statement?

ChrisCarini · 2024-06-26T11:49:40Z

parse.go

+
+	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)")


worth having this as a debug statement?

ChrisCarini · 2024-06-26T11:57:30Z

rule_id.go

+	rule.seen = nil
+	return nil


Suggested change

rule.seen = nil

return nil

rule.seen = nil

return nil

ChrisCarini · 2024-06-26T12:27:01Z

Branding values: the allowed branding values (color and icon) are currently hardcoded. I suspect, a script to extracted them from GitHub's docs should be added?

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.

ChrisCarini · 2024-06-26T12:32:43Z

Oh @msw-kialo another thing I forgot to comment on - it's worth updating the README.md as part of this change, since it's very significant, and worth highlighting this new functionality in the README. What do you think?

msw-kialo · 2024-07-01T14:10:51Z

@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:

From this version, actionlint starts to check action metadata file action.yml (or action.yaml). At this point, only very basic checks are implemented and contents of steps: are not checked yet.

Checks for steps: contents are planned to be implemented. Since several differences are expected between steps: in workflow file and steps: in action metadata file (e.g. available contexts), the implementation is delayed to later version. And the current implementation of action metadata parser is ad hoc. I'm planning a large refactorying and breaking changes Go API around it are expected.

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.

ChrisCarini · 2024-08-19T02:04:38Z

@ChrisCarini Thanks for testing it and providing feedback.

Hi @msw-kialo - no problem! Thank you for implementing this functionality and posting the PR! 🙏

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.

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.

Sam13 · 2024-10-03T08:46:03Z

Any updates on that?
Just linting workflows is not enough ;-)

msw-kialo and others added 26 commits October 9, 2023 11:15

Duplicate linting infrastructure to lint actions, too

61fecfd

For the moment we kept the old scheme and rules.

Draft action metadata parsing

dae9310

Reenable a basic visitor and rules for action linting

8992b49

Add action input parsing

6b3655c

Start parsing actions

5328f81

Resolve staticcheck linting errors

bf2f39c

Parse javascript and docker actions

cde5d87

Mimic dummy workflow/job while visits actions

f0c006c

It allows existing rules to function as are (they potentially make preparation and post actions in Workflow* Job* functions).

Parse outputs

f4693ce

Add support for action linting to playground

88bbdc2

Try to ignore broken test data for dogfooding tests

ba8491f

Fix more linting errors

8fe56c1

CI: workaround shellcheck/actionlint limitation

9f2987a

">" stricts newlines with single spaces, but apparently this isn't picked by actionlint/shellcheck

Fix playground TS linting errors

af49077

Merge branch 'rhysd:main' into lint-actions

bb3a340

Add action specific methods to visit interface

8c3038a

Starting to port rule_id, rule_expression and rule_shellcheck to correctly handle action visits.

Parse action's branding info and validate it

6cc4130

Link to right docs in comment

bb1e69d

Address TODO renaming workflowKeyVal

ef14959

Update comment

1275407

Reverse ActionCommon / Action

b95f955

Use interface for the Runs block and a normal struct for the Action itself

Unify interfaces

3bd8075

Select syntax to check based on file name or top-level yaml keys.

Cleanups and improved naming

4bf6973

More fixes

ae2dc96

Restructure / fix tests

847623f

Add more tests

4b05130

jonathan-innis mentioned this pull request Oct 27, 2023

ci: Add GHA linting aws/karpenter-provider-aws#4933

Merged

3 tasks

msw-kialo marked this pull request as ready for review October 30, 2023 14:01

jonathan-innis mentioned this pull request Oct 31, 2023

Add GHA linting kubernetes-sigs/karpenter#652

Merged

rhysd force-pushed the main branch from 0ba913c to 0ae9462 Compare February 9, 2024 15:03

rhysd force-pushed the main branch 2 times, most recently from 78e9286 to 0b49da7 Compare February 23, 2024 12:50

rhysd force-pushed the main branch from af72757 to b524f79 Compare March 5, 2024 10:41

ChrisCarini reviewed Jun 26, 2024

View reviewed changes

ChrisCarini mentioned this pull request Jun 26, 2024

Feature Request: Support linting action metadata files #46

Open

rhysd force-pushed the main branch from 492d9af to e25a8b8 Compare October 24, 2024 13:17

rhysd force-pushed the main branch 2 times, most recently from 05e056b to 5aaa4ce Compare November 12, 2024 16:13

hugo-syn mentioned this pull request Nov 20, 2024

Allow inline ignore directives to disable rule(s) for a snippet of code synacktiv/octoscan#16

Closed

rhysd force-pushed the main branch from 8806536 to d3399fe Compare December 22, 2024 11:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Lint actions #366

feat: Lint actions #366

msw-kialo commented Oct 17, 2023

msw-kialo commented Oct 30, 2023

Ezri-Mudde commented Mar 12, 2024

msw-kialo commented Mar 21, 2024

ChrisCarini left a comment

ChrisCarini Jun 26, 2024

ChrisCarini Jun 26, 2024

ChrisCarini Jun 26, 2024

ChrisCarini Jun 26, 2024

ChrisCarini Jun 26, 2024

ChrisCarini Jun 26, 2024

ChrisCarini Jun 26, 2024

ChrisCarini Jun 26, 2024

ChrisCarini Jun 26, 2024

ChrisCarini Jun 26, 2024

ChrisCarini commented Jun 26, 2024

ChrisCarini commented Jun 26, 2024

msw-kialo commented Jul 1, 2024 •

edited

Loading

ChrisCarini commented Aug 19, 2024

Sam13 commented Oct 3, 2024 •

edited

Loading

		// ActionRuns is an interface how the action is executed. Action can be executed as JavaScript, Docker or composite steps.
		type ActionRuns interface {

	// use a switch to make it a bit cleaner
	// TODO: use a switch to make it a bit cleaner

	l.debug("error occurred while visiting workflow syntax tree: %v", err)
	l.debug("error occurred while visiting action syntax tree: %v", err)

feat: Lint actions #366

Are you sure you want to change the base?

feat: Lint actions #366

Conversation

msw-kialo commented Oct 17, 2023

Included changes

Implementation decisions

msw-kialo commented Oct 30, 2023

Ezri-Mudde commented Mar 12, 2024

msw-kialo commented Mar 21, 2024

ChrisCarini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisCarini commented Jun 26, 2024

ChrisCarini commented Jun 26, 2024

msw-kialo commented Jul 1, 2024 • edited Loading

ChrisCarini commented Aug 19, 2024

Sam13 commented Oct 3, 2024 • edited Loading

msw-kialo commented Jul 1, 2024 •

edited

Loading

Sam13 commented Oct 3, 2024 •

edited

Loading