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

add zizmor for linting workflows. #2798

Merged
merged 2 commits into from
Feb 21, 2025

Conversation

NicholasTanz
Copy link
Contributor

Description of the changes being introduced by the pull request:

Fixes #2793

@NicholasTanz NicholasTanz requested a review from a team as a code owner February 20, 2025 02:54
@NicholasTanz NicholasTanz marked this pull request as draft February 20, 2025 02:54
@NicholasTanz NicholasTanz marked this pull request as ready for review February 20, 2025 03:07
Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks.

  • left one change request: the release workflow is a bit tricky to test but I'm guessing the rc release name is not correct right now. This seems like the only merge blocking thing
  • On personas: I believe "auditor" persona is not appropriate but "pedantic" might be -- feel free to experiment if you want but be aware that some of the suggestions might require discussion. Keeping the default is totally fine for this PR though
  • The output is a little verbose maybe... Can you fiddle with verbosity settings to get the output to roughly match the other tools (in tox -e lint) -- one or two lines of output from zizmor would be ideal for successful run

@NicholasTanz
Copy link
Contributor Author

  • On personas: I believe "auditor" persona is not appropriate but "pedantic" might be -- feel free to experiment if you want but be aware that some of the suggestions might require discussion. Keeping the default is totally fine for this PR though

switching to pedantic covered all of the suppressed results (5) and they seem fairly reasonable to me

4/5 of them were due to unpinned "uses", but those were all unpinned with the reason that security wasn't critical. I tried using the ignore with the trailing explanation but it didn't seem to work. (Ignoring without explanation worked fine.)

1/5 was due to excessive permissions and even though there's just one job in that workflow, I think it makes sense to specify the permissions in the job

Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM

@jku jku merged commit 39388c3 into theupdateframework:develop Feb 21, 2025
17 checks passed
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.

Lint workflows with zizmor
2 participants