Skip to content
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

Improve link checking for our docs #3022

Merged
merged 15 commits into from
May 30, 2023
Merged

Conversation

theunrepentantgeek
Copy link
Member

What this PR does / why we need it:

We have been using markdown-link-check but it has some issues specific to our context:

  • Doesn't natively handle the way Hugo transmogrifies things
    • We got close by configuring some replacement patterns, but didn't achieve full coverage
  • Slow
    • This wouldn't be a problem for smaller sites, but ours is large

In this PR, we switch to using htmltest to verify the final HTML that's generated, instead of trying to second-guess Hugo.

(Until PR#215 is merged, we're using a private fork. We'll switch once a new version is released.)

Special notes for your reviewer:

Running htmltest identified 57 broken links; fixes for those are included in this PR.

We don't need to guess at the links generated by Hugo - the built in relref function allows lookup of a page by name and will generate the appropriate link. No path is needed if the name of the page is unique (and ours mostly are). In most cases you can also omit the file extension, but it seems to be necessary in some cases.

How does this PR make you feel:
gif

If applicable:

  • this PR contains documentation

@@ -150,7 +150,7 @@ go-install kustomize sigs.k8s.io/kustomize/kustomize/v4@v4.5.7

# for docs site
go-install hugo -tags extended github.com/gohugoio/hugo@v0.88.1
go-install htmltest github.com/wjdp/htmltest@v0.15.0
go-install htmltest github.com/theunrepentantgeek/htmltest@latest
Copy link
Collaborator

@super-harsh super-harsh May 29, 2023

Choose a reason for hiding this comment

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

Leave a todo here mentioning the reason and parent repo

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member Author

Choose a reason for hiding this comment

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

I also created #3024 so we don't lose track of the need to update.

cmds:
# Excluding './content/reference/*' path as it contains all auto-generated API docs.
- find . -type f -name "*.md" -not -path "./content/reference/*" -not -path "./node_modules/*" -print0 | xargs -0 -n1 markdown-link-check -c link-checker.json
- htmltest
Copy link
Collaborator

Choose a reason for hiding this comment

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

how does this work? do we not require to provide any path?

Copy link
Member Author

Choose a reason for hiding this comment

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

The task runs in docs/hugo (see line 90) and there's a .htmltest.yml file in that folder that has the rest of the configuration.

@codecov-commenter
Copy link

codecov-commenter commented May 29, 2023

Codecov Report

Merging #3022 (1c227b9) into main (ad02e47) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #3022      +/-   ##
==========================================
- Coverage   54.20%   54.20%   -0.01%     
==========================================
  Files        1374     1374              
  Lines      580799   580799              
==========================================
- Hits       314813   314801      -12     
- Misses     214198   214205       +7     
- Partials    51788    51793       +5     

see 5 files with indirect coverage changes

Copy link
Collaborator

@super-harsh super-harsh left a comment

Choose a reason for hiding this comment

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

LGTM! just a few minors. Also, get rid of link-checker.json file as we don;t use it anymore.

@@ -1,3 +1,13 @@
DirectoryPath: public
CheckExternal: false
Copy link
Collaborator

Choose a reason for hiding this comment

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

We do want to check the external links as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll do that in a separate PR.

@theunrepentantgeek theunrepentantgeek merged commit 5c85fde into main May 30, 2023
9 checks passed
@theunrepentantgeek theunrepentantgeek deleted the improve/link-checking branch May 30, 2023 04:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants