Skip to content

Add zizmor as gh action - #3482

Open
EmmanuelNiyonshuti wants to merge 6 commits into
python-trio:mainfrom
EmmanuelNiyonshuti:harden-ci
Open

Add zizmor as gh action#3482
EmmanuelNiyonshuti wants to merge 6 commits into
python-trio:mainfrom
EmmanuelNiyonshuti:harden-ci

Conversation

@EmmanuelNiyonshuti

Copy link
Copy Markdown
Contributor

Closing #3168 and following up on #3154 (comment)

@github-advanced-security

Copy link
Copy Markdown

You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool.

What Enabling Code Scanning Means:

  • The 'Security' tab will display more code scanning analysis results (e.g., for the default branch).
  • Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results.
  • You will be able to see the analysis results for the pull request's branch on this overview once the scans have completed and the checks have passed.

For more information about GitHub Code Scanning, check out the documentation.

@A5rocks A5rocks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! Maybe it's a good idea to remove zizmor from pre-commit (if we have it there), to keep the versions identical. Also, maybe it would be nice to require the zizmor action? I.e. put it in the ci.yml file and as a requirement to the required workflow.

But not completely sure about either idea...

@codecov

codecov Bot commented Jul 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00000%. Comparing base (72abfa5) to head (e580c39).

Additional details and impacted files
@@               Coverage Diff               @@
##                 main        #3482   +/-   ##
===============================================
  Coverage   100.00000%   100.00000%           
===============================================
  Files             128          128           
  Lines           19449        19449           
  Branches         1320         1320           
===============================================
  Hits            19449        19449           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@EmmanuelNiyonshuti

Copy link
Copy Markdown
Contributor Author

Pointing all our actions to zizmor passes on all zizmor operating modes🎉, we also have it in pre-commit, so it was straightforward to add the action. I don't know much about this tool but there probably are still room for improvements like I saw that attrs uses a pedantic persona https://github.com/python-attrs/attrs/blob/main/.github/workflows/zizmor.yml#L31

more on personas: https://docs.zizmor.sh/usage/#using-personas

@EmmanuelNiyonshuti

Copy link
Copy Markdown
Contributor Author

Thanks! Maybe it's a good idea to remove zizmor from pre-commit (if we have it there), to keep the versions identical. Also, maybe it would be nice to require the zizmor action? I.e. put it in the ci.yml file and as a requirement to the required workflow.

But not completely sure about either idea...

IMO i don't think its a bad idea to require it in ci.yml BUT i have never seen it in other projects so far, I think I didn't see it in zizmor itself too? not sure though plus I don't understand the need beyond what it would do if not required in ci.yml.

@A5rocks

A5rocks commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

IMO i don't think its a bad idea to require it in ci.yml BUT i have never seen it in other projects so far, I think I didn't see it in zizmor itself too?

To be clear I'm talking about how we have a single workflow (it uses alls-green but I forgot exact naming...) required (so that changes to required workflows can be done via PR rather than through the repo settings). I guess we need to put this in ci.yml to make the zizmor run required?

@EmmanuelNiyonshuti

Copy link
Copy Markdown
Contributor Author

IMO i don't think its a bad idea to require it in ci.yml BUT i have never seen it in other projects so far, I think I didn't see it in zizmor itself too?

To be clear I'm talking about how we have a single workflow (it uses alls-green but I forgot exact naming...) required (so that changes to required workflows can be done via PR rather than through the repo settings). I guess we need to put this in ci.yml to make the zizmor run required?

Sorry, i may have slightly misunderstood what you was suggesting. I think the job you are referring to is this one, no?

# https://github.com/marketplace/actions/alls-green#why
check: # This job does nothing and is only used for the branch protection

and I can call zizmor from here as well.

Comment thread .github/workflows/ci.yml Outdated
Comment thread .github/workflows/ci.yml Outdated
Comment thread .github/workflows/ci.yml Outdated
Comment on lines +480 to +485
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
with:
persist-credentials: false

- name: Run zizmor 🌈
uses: zizmorcore/zizmor-action@6fc4b006235f201fdab3722e17240ab420d580e5 # v0.6.1

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

do we have a bot/workflow that will update these pinned versions? Pinning it to a specific hash otherwise seems like it'll quickly go stale. When we had it in pre-commit it was handled by the pre-commit bot.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

perhaps related, why does this specify # v0.6.1 when the pre-commit was using v1.28.0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, I guess non-hashes would be better for now. We want to eventually switch to locked actions (whether that's through hashes or through GitHub's native thing (coming soon:tm:)...), at which point we can figure out updating :-)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Granted: It was almost copy pasted from zizmor integration docs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

perhaps related, why does this specify # v0.6.1 when the pre-commit was using v1.28.0

# v0.6.1 is the action's version and the latter was for the zizmor version used by pre-commit?

@EmmanuelNiyonshuti

Copy link
Copy Markdown
Contributor Author

the ci failures seems related to codecov. I will investigate.

@CoolCat467

Copy link
Copy Markdown
Member

Given running things on pre-commit.ci is free, why drop running pre-commit hook version of zizmor? In the event someone changes a github action and something breaks, if they do pre-commit checks it'll save them a whole cycle of looking at github action logs on github. If we don't want pre-commit.ci to run zizmor and only run locally, we can just add zizmor to the ci.skip section in pre-commit.

I will admit though, this will introduce version matching issues, would likely want a tool similar to sync_requirements but changing the github actions version instead or just adding this functionality onto the existing tool.

@A5rocks

A5rocks commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

I suggested it cause the version matching, yeah. (and also since the action will be able to check more since it has a github token, so passing one doesn't mean passing the other)

I'd be fine with whatever outcome is best.

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.

5 participants