-
Notifications
You must be signed in to change notification settings - Fork 881
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
[enhancement]: Automatically linkcheck in CI #4479
[enhancement]: Automatically linkcheck in CI #4479
Conversation
Signed-off-by: Aviral Singh <[email protected]>
Signed-off-by: Aviral Singh <[email protected]>
Signed-off-by: Aviral Singh <[email protected]>
Signed-off-by: Aviral Singh <[email protected]>
Signed-off-by: Aviral Singh <[email protected]>
Signed-off-by: Aviral Singh <[email protected]>
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 for this first contribution to cloud-init @itsaviral2609!
I've confirmed you have signed the CLA and have a minor suggestion or two to discuss here.
Our linkchecker target seems to raise a bunch of errors on links which shouldn't be valid, and some links we shouldn't be checking every time we run CI.
I think it's worth adding the following to our sphinx conf to ignore a few things:
diff --git a/doc/rtd/conf.py b/doc/rtd/conf.py
index fcb8f2e30..5c9be14a6 100644
--- a/doc/rtd/conf.py
+++ b/doc/rtd/conf.py
@@ -119,6 +119,16 @@ html_extra_path = ["googleaf254801a5285c31.html"]
autosectionlabel_prefix_document = True
autosectionlabel_maxdepth = 2
+# Sphinx-linkcheck config
+linkcheck_ignore = [
+ r"http://instance-data.*", r"https://powersj.io.*",
+ r"http://169.254.169.254.*", r"http://10.10.0.1.*"
+]
+linkcheck_anchors_ignore_for_url = (
+ r"https://github.com/canonical/cloud-init.*",
+ r"https://github.com/canonical/ubuntu-pro-client.*"
+)
+
# Sphinx-copybutton config options:
notfound_body = (
"<h1>Page not found</h1><p>Sorry we missed you! Our docs have had a"
failOnError: | ||
description: 'Fail job on link check error' | ||
required: false | ||
default: 'false' |
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 for the constructive workflow_dispatch suggestion here for readability of continue-on-error below.
.github/workflows/linkcheck.yml
Outdated
run: pip install tox | ||
|
||
- name: Run Sphinx linkcheck | ||
run: tox -e linkcheck |
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.
For the moment it looks like we are going to have a few broken links, hence good to have the continue-on-error: false
. I think it may be worth grep broken
on the linkcheck and tee int the output of broken lines to another file and performing a line-count and assert that the broken link line count is < 6.
This way we could set a high-water mark that we can reduce in subsequent PRs as we sort external stale documentation links and update our docs.
Signed-off-by: Aviral Singh <[email protected]>
@blackboxsw I have made the requested changes! |
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.
Looks great @itsaviral2609. Last review round and we'll turn on CI and land this branch, many thanks.
doc/rtd/conf.py
Outdated
@@ -119,6 +119,15 @@ | |||
autosectionlabel_prefix_document = True | |||
autosectionlabel_maxdepth = 2 | |||
|
|||
# Sphinx-linkcheck config | |||
linkcheck_ignore = [ r"http://instance-data.*", r"https://powersj.io.*", |
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.
Great thanks @itsaviral2609, one more ignore to add and then I think we can drop the high-water mark for # of broken links from 6 to 3
diff --git a/doc/rtd/conf.py b/doc/rtd/conf.py
index 0dc3b4839..03c8688d2 100644
--- a/doc/rtd/conf.py
+++ b/doc/rtd/conf.py
@@ -120,8 +120,9 @@ autosectionlabel_prefix_document = True
autosectionlabel_maxdepth = 2
# Sphinx-linkcheck config
-linkcheck_ignore = [ r"http://instance-data.*", r"https://powersj.io.*",
- r"http://169.254.169.254.*", r"http://10.10.0.1.*"
+linkcheck_ignore = [
+ r"http://\[fd00:ec2::254.*", r"http://instance-data.*",
+ r"https://powersj.io.*", r"http://169.254.169.254.*", r"http://10.10.0.1.*"
]
linkcheck_anchors_ignore_for_url = (
r"https://github.com/canonical/cloud-init.*",
.github/workflows/linkcheck.yml
Outdated
- name: Check for broken links below threshold 6 | ||
run: | | ||
broken_count=$(grep -c "broken" output.txt) | ||
if [[ $broken_count -ge 6 ]]; then |
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.
Thank your for this extra work here. This looks good. We also want to avoid the internal ec2 ipv6 instance metadata service r"http://\[fd00:ec2::254.*"
and we should be good to drop the high-water mark from 6 to 3 and merge this PR. Thanks again for the swift work here.
if [[ $broken_count -ge 6 ]]; then | |
if [[ $broken_count -ge 3 ]]; then |
Signed-off-by: Aviral Singh <[email protected]>
Done with the requested changes! I'm working on issue #4453 too and was facing issues to generate template while running this command I will integrate this issue to CI too if I get more clear objectives of this task @blackboxsw |
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.
Looks good to me. Welcome to cloud-init! Pushed a commit that fixes black formatting style on conf.py, by running: tox -e do_format on this branch. I'll land when CI is happy
fixed using tox -e do_format
Proposed Commit Message
workflow for linkcheck in CI
Note for Maintainers
Workflow subject to changes by maintainers.
Fixes GH- #4460
Checklist: