-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
CI: Link checker script #27116
CI: Link checker script #27116
Conversation
428b71f
to
a94ec93
Compare
@qmonnet Please let me know your thoughts about this one. Appreciate the help ! |
The change looks good, now we need to see how it looks like in practice in the CI:
|
Turning to draft given it's blocked on #26880 for now. |
Documentation workflow took 11 minutes to complete. I hope it gets faster once more links are fixed and it doesn't have to wait for timeouts. |
a94ec93
to
d7e62ce
Compare
@qmonnet Need some help regarding fixing the issue:
I went through the issues in the docutils and found this: https://sourceforge.net/p/docutils/bugs/459/ . It seems to have been resolved in version 0.18 of docutils and only exixts in version 0.17. |
I'm not sure we can update to Docutils 0.18 yet, last time I checked we were still stuck with some dependencies on 0.17 somewhere. Possibly for the theme, I don't think we've rebased our fork on top of readthedocs/sphinx_rtd_theme#1381 yet. If it's just dependent on 0.17, let's filter out this warning with a comment in the script telling to remove it after we've upgraded to a newer docutils version? |
3efb255
to
0208bdb
Compare
f76a2a0
to
52faa71
Compare
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.
Looking at my latest run, it was maybe not the failure()
that blocked the artifact download, but possibly the needs.links-checker.outputs.status == 'failure'
instead: I don't think we're ever setting this output for links-checker
? Looking at a similar usage of outputs.status
in file .github/workflows/conformance-ginkgo.yaml
, there is echo "status=failure" >> $GITHUB_OUTPUT
to set the status, it's not automatic. So we probably need something similar.
Annoyingly, the link checker step also failed (consistently, it seems) on this new run, and I've no idea what's causing it:
[...]
reading sources... [ 58%] gettingstarted/hubble_setup
reading sources... [ 58%] gettingstarted/k8s-install-default
Please fix the following documentation warnings:
Exception occurred:
File "/usr/local/lib/python3.11/site-packages/docutils/parsers/rst/states.py", line 2297, in comment
self.document.include_log.pop()
IndexError: pop from empty list
The full traceback has been saved in /tmp/sphinx-err-1xnht2gf.log, if you want to report the issue to the developers.
Please also report this if it was a user error, so that a better error message can be provided next time.
A bug report can be filed in the tracker at <https://github.com/sphinx-doc/sphinx/issues>. Thanks!
Adding the warnings to the file.
Did you observe this before, by any chance?
I can reproduce the link checker issue locally and dump the logs, but that's not super useful. Looks like a Sphinx bug (we recently bumped the version and updated the
Logs from Sphinx
|
Looks like a bug caused by the bump of docutils from 0.17.1 to 0.18.1 - bisecting shows it is introduced by https://repo.or.cz/docutils.git/commitdiff/93dc79108625a3af6be11a3eb85e50be59b2c910?hp=0515f37ca519ecfde133040425a06b6e4887aea0. Not sure how to handle this, though 🤔 |
Ping, anything we can do to move this along? |
Thanks Timo. I must admit I'm a bit stuck here. I talked with @zacharysarah about this PR, and we don't see a good path forward. This bug in the link checker is clearly an issue. We know the cause, but it's tricky to address.
Add to that that there are internal discussions about moving away from Sphinx in the future (but nothing decided yet), so there's little appetite for long-term commitments/solutions at the moment. Maybe the simplest option for now would be to drop the CI workflow (sorry Vipul 😢), and try to merge |
I can remove the workflow and update the script, if that is the suggested path forward. Let me know. |
a13bb95
to
966d910
Compare
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.
The changes look all good, thanks a lot.
On the second commit, can you please:
- Update the commit title, given that we no longer enable the link checker in the CI
- Add a commit description that summarises what we're doing here (and why we don't enable it in CI)
- Drop the
Co-authored-by:
tag, or updated it with my work email address (<quentin@isovalent.com>
)
We should be all good after that
Link checker is separated out from the check-build.sh into a separate script. It creates a separate file with the broken links and filters based on the known/expected errors. It is not added to the github workflow as there is a known issue in the docutils pkg used. (Issue: cilium#27116 (comment)) Co-authored-by: Quentin Monnet <quentin@isovalent.com> Signed-off-by: Vipul Singh <vipul21sept@gmail.com>
966d910
to
f27fdb8
Compare
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.
Thanks a lot for this work!
/test |
Thanks for the contribution, merged! |
Please ensure your pull request adheres to the following guidelines:
description and a
Fixes: #XXX
line if the commit addresses a particularGitHub issue.
Fixes: <commit-id>
tag, thenplease add the commit author[s] as reviewer[s] to this issue.
Enabling the link checker for the CI.
ref.myst
in the suppress_warning for sphinx.Note: This is dependent on merge of PR: CI: Links in docs are not tested #10601 to make sure CI passes
Related: #10601