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

🌱 Switch link checker to mdbook-linkcheck #3474

Merged
merged 1 commit into from
Sep 22, 2020

Conversation

benmoss
Copy link

@benmoss benmoss commented Aug 11, 2020

What this PR does / why we need it:
Switches our link checker from https://github.com/raviqqe/liche to https://github.com/Michael-F-Bryan/mdbook-linkcheck/

This tool is directly integrated with mdbook which is nice, since it tests against rendered the rendered Markdown with our preprocessors rather than just scanning the raw files. It also supports adding headers for specific domains, which helps with avoiding rate-limiting by GitHub. It has built-in caching too, so links that have already been verified are faster.

I'm using my fork of https://github.com/japaric/trust/ which just gives us a nice succinct script to download Rust GitHub releases. We could also vendor the script. The upstream trust repo has a problem in that it requires rustc even if you're just using it to download a GitHub release, which I've made a PR to fix here.

Removing verify-book-links as a part of make verify so that it doesn't become PR-blocking. I'm going to make a separate PR to test-infra to make it just a separate optional job, which for now should give us good enough visibility if a PR breaks a link or if a link breaks due to reasons outside our control (external servers changing).

Part of #2703

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 11, 2020
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 11, 2020
@benmoss benmoss force-pushed the verify-links branch 2 times, most recently from e05180a to 8366b8a Compare August 11, 2020 14:22
@benmoss
Copy link
Author

benmoss commented Aug 26, 2020

We could add GITHUB_TOKEN to the Netlify environment variables to avoid hitting rate-limits https://docs.netlify.com/configure-builds/environment-variables/#deploy-request-notifications but I think we'd be blocking PRs if links fail for unrelated reasons.

I'll remove the link-checking behavior from the netlify deploy for now.

@benmoss benmoss force-pushed the verify-links branch 2 times, most recently from 2d8a99a to 7e9373b Compare August 26, 2020 15:14
docs/book/Makefile Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 26, 2020
docs/book/Makefile Outdated Show resolved Hide resolved
@vincepri
Copy link
Member

vincepri commented Sep 3, 2020

/milestone v0.3.10

@k8s-ci-robot k8s-ci-robot added this to the v0.3.10 milestone Sep 3, 2020
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 18, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 21, 2020
@vincepri
Copy link
Member

/milestone Next

@benmoss
Copy link
Author

benmoss commented Sep 21, 2020

@vincepri any reason we keep pushing this one off? It's catching regressions we're missing

@vincepri
Copy link
Member

Happy to merge whenever it's ready

@benmoss
Copy link
Author

benmoss commented Sep 21, 2020

Should be ready now, I hadn't noticed the netlify failure

@benmoss
Copy link
Author

benmoss commented Sep 22, 2020

@vincepri should be ready to go now, or at least review 😄

Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/approve
/hold cancel
/assign @ncdc @detiber

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 22, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@vincepri
Copy link
Member

/milestone v0.3.10

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 22, 2020
@k8s-ci-robot k8s-ci-robot modified the milestones: Next, v0.3.10 Sep 22, 2020
@ncdc
Copy link
Contributor

ncdc commented Sep 22, 2020

Do we want the deploy/netlify preview to fail if there are broken links? It's not currently because the linkcheck is marked as optional and our netlify configuration doesn't install the link checker as part of the build.

@ncdc
Copy link
Contributor

ncdc commented Sep 22, 2020

@detiber WDYT?

@detiber
Copy link
Member

detiber commented Sep 22, 2020

I'm a bit torn on failing the build... In one sense it would be nice to get a heads up that things are broken and/or verify any new links added are not broken, but it also seems a bit harsh to penalize a contributor's PR if an existing link has become broken.

Unless there is a way to differentiate the two, then I think it should likely remain optional.

@ncdc
Copy link
Contributor

ncdc commented Sep 22, 2020

/lgtm
😄

That's fair. How do we anticipate getting signal that links are broken?

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 22, 2020
@detiber
Copy link
Member

detiber commented Sep 22, 2020

That's fair. How do we anticipate getting signal that links are broken?

For netlify notifications, it looks like it is possible to configure both Slack and email notifications.

For slack, it would require getting a slack token: https://github.com/kubernetes/community/blob/13b3f6a596f3b31c668409c1a52f386f98eab93f/.github/ISSUE_TEMPLATE/slack-request.md

@k8s-ci-robot k8s-ci-robot merged commit 9809625 into kubernetes-sigs:master Sep 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants