-
Notifications
You must be signed in to change notification settings - Fork 745
Lint Github Actions workflows with zizmor #3188
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
base: devel
Are you sure you want to change the base?
Changes from 10 commits
7830f43
c920ae8
7fd909e
05a1858
234868e
3623b3f
1656b3f
5f38280
116dce7
062e554
041e7a0
5130b83
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,3 +9,5 @@ updates: | |
| directory: "/" | ||
| schedule: | ||
| interval: "weekly" | ||
| cooldown: | ||
| default-days: 4 | ||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -32,9 +32,6 @@ name: Build docs | |||||||
| DOCS_BOT_TOKEN: | ||||||||
| required: true | ||||||||
|
|
||||||||
| env: | ||||||||
| PACKAGE_VERSION: ${{ inputs.ansible-package-version }} | ||||||||
|
|
||||||||
| jobs: | ||||||||
| build-package-docs: | ||||||||
| runs-on: ubuntu-latest | ||||||||
|
|
@@ -50,26 +47,34 @@ jobs: | |||||||
| }} | ||||||||
| ref: ${{ inputs.repository-branch }} | ||||||||
| path: build-directory | ||||||||
| persist-credentials: false | ||||||||
|
|
||||||||
| - name: Setup nox | ||||||||
| uses: wntrblm/nox@2025.10.16 | ||||||||
|
|
||||||||
| - name: Output Python info | ||||||||
| run: python --version --version && which python | ||||||||
|
|
||||||||
| - name: Set the VERSION variable | ||||||||
| run: echo VERSION="${PACKAGE_VERSION}" >> "${GITHUB_ENV}" | ||||||||
| - name: Graft ansible-core | ||||||||
| run: nox -s clone-core | ||||||||
| working-directory: build-directory | ||||||||
|
|
||||||||
| - name: Build the Ansible community package docs | ||||||||
| run: >- | ||||||||
| nox -e make -- webdocs ${{ | ||||||||
| env: | ||||||||
| PACKAGE_VERSION: "${{ inputs.ansible-package-version }}" | ||||||||
|
||||||||
| PACKAGE_VERSION: "${{ inputs.ansible-package-version }}" | |
| PACKAGE_VERSION: "${{ inputs.ansible-package-version }}" | |
| VERSION: "${{ inputs.ansible-package-version }}" |
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.
Hi @gotmax23 ANSIBLE_VERSION gets passed to the Makefile but not VERSION.
I think VERSION is a relic from the Jenkins job that is used in a couple of places including the step to add a symlink on the web server: ln -s \$VERSION _build/latest. Maybe we needed it when we first started with the workflow but it should be fine to drop it as part of this PR.
I'm pretty sure I built this branch before I approved but will do it again now. Thanks, as always, for the thoughtful questions.
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.
Just did a run with this branch from the devel branch and see this in the logs: nox > make -C docs/docsite PYTHON=/home/runner/work/ansible-documentation/ansible-documentation/build-directory/.nox/make/bin/python webdocs ANSIBLE_VERSION= 'EXTRA_TAGS=-t redirects'
Everything in the tarball looks good too. I think we can safely remove the Set the VERSION variable step.
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 see
$ rg '^VERSION.*=' docs/docsite/Makefile
VERSION := $(shell $(PYTHON) ./version_helper.py --raw || echo error). Assuming I remember how Makefile syntax works, I think this will set VERSION to a default value unless it is already set. VERSION was previously set in the environment, but now it's not, so I think that the value generated by that script would be used instead. I'm not sure what impact that has.
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.
Hmm. I thought that line was more related to how the core versions get set in docs. But it's true that VERSION no longer gets set in the environment.
If there's any impact it would be to the version list drop down, which I can see is still at 11 here: https://docs.ansible.com/projects/ansible/devel/index.html
Perhaps we stomped on how VERSION was getting set when we made this a reusable workflow and it's been broken? I'll go back and look. I've compared the generated HTML built from this branch with what we're building on devel and they seem to match. So I really don't think we're going to break something new by dropping VERSION.
I'm also not sure it's worth fixing whatever VERSION was doing for the package docs. We've got the Read The Docs flyout menu on docs.ansible.com now. Part of the migration effort was to adopt RTD native tooling instead of the custom version switcher. That might be a good step towards decoupling package and core docs, which is also the root of some of the complexity in the Makefile.
gotmax23 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
gotmax23 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
felixfontein marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,10 +34,14 @@ jobs: | |
| - session: "pip-compile" | ||
| extra-args: "--check" | ||
| python-versions: "3.12" | ||
| - session: "zizmor" | ||
| python-versions: "3.12" | ||
|
Comment on lines
+37
to
+38
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could potentially remove this in favor of the workflow added in #3141 and just keep the zizmor in the noxfile for contributors to run locally. Although, I'm concerned that:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The zizmor workflow not failing CI seems to be by design of the GHA author, keeping that data in the security section for action separate from success/failure of the CI itself. As for not being able to find the reported issues, that was a "I needed to learn how to use the tools together". I was running the scans on one branch, and the scan was showing data from another, and I just missed how I could adjust that to see the results (all of that was happening in my own fork). With all that being said, since a majority of the workflows for the project are setup using nox to make them portable between dev/CI environments, I can't think of any good objections to integrating it there. I initially went the GHA route because it appears to be the suggested method by the tool authors, with the intention of piping that data automatically into existing GH systems (which does work as intended, but doesn't sound like its exactly the use case we're targeting).
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gotmax23 that workflow should be complementary as the nox session won't integrate with GH through SARIF. Just add another job separately. @x1101 I believe that once merged, the issues would show up in PRs inline. But it's a good idea to have both invocation mechanisms in place. The reusable workflow is for the GH Code Scanning integration while nox is for local use. However, it's a good idea to still run that nox session in CI to ensure that it works in dev env.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gotmax23 FYI the code scanning results don't normally show up until the workflow reporting them is in |
||
| name: "Run nox ${{ matrix.session }} session" | ||
| steps: | ||
| - name: Check out repo | ||
| uses: actions/checkout@v5 | ||
| with: | ||
| persist-credentials: false | ||
| - name: Setup nox | ||
| uses: wntrblm/nox@2025.10.16 | ||
| with: | ||
|
|
@@ -46,7 +50,8 @@ jobs: | |
| run: | | ||
| nox -e clone-core | ||
| - name: "Run nox -e ${{ matrix.session }}" | ||
| # Using GHA expression interpolation is fine here, | ||
| # as we control all the inputs. | ||
| # zizmor: ignore[template-injection] | ||
| run: | | ||
| # Using GHA expression interpolation is fine here, | ||
| # as we control all the inputs. | ||
| nox -e "${{ matrix.session }}" -- ${{ matrix.extra-args }} | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -35,23 +35,21 @@ name: "Refresh pinned dependencies" | |||||||||||||||||||||
| python-versions: | ||||||||||||||||||||||
| type: string | ||||||||||||||||||||||
| required: true | ||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| permissions: | ||||||||||||||||||||||
| contents: read | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| jobs: | ||||||||||||||||||||||
| refresh: | ||||||||||||||||||||||
| runs-on: ubuntu-latest | ||||||||||||||||||||||
| environment: github-bot | ||||||||||||||||||||||
| steps: | ||||||||||||||||||||||
| - name: Generate temp GITHUB_TOKEN | ||||||||||||||||||||||
| id: create_token | ||||||||||||||||||||||
| uses: actions/create-github-app-token@v2 | ||||||||||||||||||||||
| with: | ||||||||||||||||||||||
| app-id: ${{ secrets.BOT_APP_ID }} | ||||||||||||||||||||||
| private-key: ${{ secrets.BOT_APP_KEY }} | ||||||||||||||||||||||
| - name: Check out repo | ||||||||||||||||||||||
| uses: actions/checkout@v5 | ||||||||||||||||||||||
| with: | ||||||||||||||||||||||
| fetch-depth: 0 | ||||||||||||||||||||||
| ref: "${{ inputs.base-branch }}" | ||||||||||||||||||||||
| token: "${{ steps.create_token.outputs.token }}" | ||||||||||||||||||||||
| persist-credentials: false | ||||||||||||||||||||||
| - name: Fetch required contents of ansible-core | ||||||||||||||||||||||
| run: | | ||||||||||||||||||||||
| python docs/bin/clone-core.py | ||||||||||||||||||||||
|
|
@@ -82,9 +80,23 @@ jobs: | |||||||||||||||||||||
| env: | ||||||||||||||||||||||
| # Ensure the latest pip version is used | ||||||||||||||||||||||
| VIRTUALENV_DOWNLOAD: '1' | ||||||||||||||||||||||
| # | ||||||||||||||||||||||
| # nox-args is defined statically in the calling workflows and passing it | ||||||||||||||||||||||
| # as a shell variable presents quoting issues, so ignore zizmor error | ||||||||||||||||||||||
| # for now. | ||||||||||||||||||||||
| # zizmor: ignore[template-injection] | ||||||||||||||||||||||
| run: | | ||||||||||||||||||||||
| nox ${{ inputs.nox-args }} | ||||||||||||||||||||||
| - name: Generate temp GITHUB_TOKEN | ||||||||||||||||||||||
| id: create_token | ||||||||||||||||||||||
| uses: actions/create-github-app-token@v2 | ||||||||||||||||||||||
| with: | ||||||||||||||||||||||
| app-id: ${{ secrets.BOT_APP_ID }} | ||||||||||||||||||||||
| private-key: ${{ secrets.BOT_APP_KEY }} | ||||||||||||||||||||||
| # We could rely on the checkout action to persist the token in the | ||||||||||||||||||||||
| # repository config, but this way, we can prevent the previous steps | ||||||||||||||||||||||
| # from having unnecessary access. | ||||||||||||||||||||||
| - name: "Set up token authentication" | ||||||||||||||||||||||
| run: gh auth setup-git --force --hostname github.com | ||||||||||||||||||||||
| - name: Push new dependency versions and create a PR | ||||||||||||||||||||||
| env: | ||||||||||||||||||||||
| GITHUB_TOKEN: ${{ steps.create_token.outputs.token }} | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| rules: | ||
| unpinned-uses: | ||
| config: | ||
| policies: | ||
| "wntrblm/nox": ref-pin | ||
| "re-actors/alls-green": ref-pin | ||
| "actions/*": ref-pin |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| zizmor |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| # This file was autogenerated by uv via the following command: | ||
| # uv pip compile --universal --output-file tests/zizmor.txt tests/zizmor.in | ||
| zizmor==1.16.0 | ||
| # via -r tests/zizmor.in |
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.
That workflow should declare an explicit
secrets:section. Then, passing secrets will work. Just likeinputs:.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.
Right, but the secrets are part of the
github-botenvironment, and I don't thinkenvironmentcan be set in the calling workflow.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.
Oh, so https://docs.github.com/en/actions/how-tos/reuse-automations/reuse-workflows#using-inputs-and-secrets-in-a-reusable-workflow implies that the environment should be set in the reusable workflow but doesn't explicitly talk about how that interacts with the secrets key (beyond the inability to pass them explicitly).