-
Notifications
You must be signed in to change notification settings - Fork 352
-
Following up on #2160 (comment)
The tuple (ubuntu-latest, 3.12) is odd... where is that coming from? All of the other a11y-tests are labeled as (OS, browser), so 3.12, which I'm sure is a reference to the Python version, doesn't make any sense to me.
It only occurred to me that this is a remnant of the repository workflow enforcement rules.
When updating the CI workflows in #2077 to include more OS targets for the accessibility tests, I did not update these workflows, as this functionality has now moved to Rulesets as Status checks (ref: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/collaborating-on-repositories-with-code-quality-features/about-status-checks).
Since we were due a migration to rulesets from the now deprecated functionality, I created a new pr-quality-check ruleset that requires:
- 1 PR approval
- CI status checks:
pytest: all combinations (OS, Sphinx, Python version)- Doc build checks: all combinations (OS, Sphinx, Python version)
- RTD preview
- lighthouse-audit
- a11y: ubuntu-latest, ubuntu-22.04, chromium, firefox (since the MacOS has been flaky, I left it out for now.
This should hopefully remove the old checks needed that are showing in the UI right now 🤞🏽
If there is anything I missed, please LMK @drammock @gabalafou
Beta Was this translation helpful? Give feedback.
All reactions
-
👍 1
Replies: 2 comments 5 replies
-
Thanks for wrangling the CIs (again). I haven't had the bandwidth to dive in recently.
So IIUC, based on what you said above the following CI actions/jobs are not required to pass for merge:
- zizmor
- linkcheck
- coverage
- profiling
My (not expert) opinion would be to make zizmor required (since it's security-focused) and also linkcheck (because it rarely fails and is usually easy to fix when it does). Coverage has historically been flaky (though maybe not anymore since we switched to the coverage action instead of codecov?) so I'm OK with not requiring that one for now. I think I've never even looked at profiling 🙈.
I browsed the "Checks" tab on #2163, just to familiarize myself with how the results look in that UI. I see a warning annotation on Zizmor workflow:
No file matched to [**/uv.lock,**/requirements*.txt]. The cache will never get invalidated. Make sure you have checked out the target repository and configured the cache-dependency-glob input correctly.
I also (oddly) see both a Zizmor workflow, and a zizmor job listed under "Code Scanning Results". Maybe that's a quirk of the UI, but we should check that we're not running it twice.
I also noticed several annotations from the "check coverage" action in the "Continuous Integration" workflow; the salient ones are these:
- a couple of "HTTP/1.1 403 Forbidden"
- "Cannot post comment. This is probably because this is an external PR, so it's expected. Ensure you have an additional
workflow_runstep configured as explained in the documentation (or alternatively, give up on PR comments for external PRs)."
Beta Was this translation helpful? Give feedback.
All reactions
-
Thanks for wrangling the CIs (again). I haven't had the bandwidth to dive in recently.
Happy to help!
@drammock yes I was on the fence re Zizmor, but I agree with you, it should be required.
Re the annotation
No file matched to [/uv.lock,/requirements*.txt]. The cache will never get invalidated. Make sure you have checked out the target repository and configured the cache-dependency-glob input correctly.
I will have a look into it, there should be no cache there but it seems uv is not too happy about this.
I also (oddly) see both a Zizmor workflow, and a zizmor job listed under "Code Scanning Results". Maybe that's a quirk of the UI, but we should check that we're not running it twice.
We only run the Zizmor workflow once, and the results are uploaded as sarif which makes them available via the Code Scanning results, so it is only extra output.
I also noticed several annotations from the "check coverage" action in the "Continuous Integration" workflow; the salient ones are these:
a couple of "HTTP/1.1 403 Forbidden"
"Cannot post comment. This is probably because this is an external PR, so it's expected. Ensure you have an additional workflow_run step configured as explained in the documentation (or alternatively, give up on PR comments for external PRs)."
This should be fixed in #2162
I changed from a workflow_run to a workflow_call after adding Zizmor which recommends this, but in doing so I missed updating the permissions.
Beta Was this translation helpful? Give feedback.
All reactions
-
@drammock re the linkcheck workflow. I have noticed some timeout-related errors like https://github.com/pydata/pydata-sphinx-theme/actions/runs/13813331844/job/38640040791?pr=2162
dispute the links being valid.
I might be able to update linkcheck's configuration as this only seems to happen with the gnu gettext site.
Shall I make this required still, or shall we wait until I (or someone) else figures out what is going on with this?
Beta Was this translation helpful? Give feedback.
All reactions
-
see my other comment related to linkcheck: #2162 (comment) I think let's NOT make it required until we get that sorted out, and maybe also wait until we configure it to have a longer timeout / more retries for GNU gettext.
Beta Was this translation helpful? Give feedback.
All reactions
-
👍 1
-
Sounds like a plan
Beta Was this translation helpful? Give feedback.
All reactions
-
since the MacOS has been flaky, I left it out for now.
Does that mean it will still run on PRs but won't be required?
Beta Was this translation helpful? Give feedback.
All reactions
-
That's right, for now it runs on all the PRs but will not block a merge if it fails.
Your fixes already fixed this. But to be on the safe side and avoid having to bypass rules, I might only enable it in a few days as required.
Beta Was this translation helpful? Give feedback.