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

feat: Lint actions #366

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

feat: Lint actions #366

wants to merge 26 commits into from

Conversation

msw-kialo
Copy link

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?

msw-kialo and others added 26 commits October 9, 2023 11:15
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 msw-kialo marked this pull request as ready for review October 30, 2023 14:01
@msw-kialo
Copy link
Author

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

@Ezri-Mudde
Copy link

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
Copy link
Author

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.

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.

None yet

3 participants