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

Lint markdown files for broken links. #497

Merged
merged 4 commits into from
Dec 8, 2023
Merged

Lint markdown files for broken links. #497

merged 4 commits into from
Dec 8, 2023

Conversation

anybodys
Copy link
Contributor

@anybodys anybodys commented Dec 7, 2023

Ticket

Resolves #490

Changes

  • Add a lint make target for markdown files. Add a linter that checks for broken links.
  • Fix broken link in markdown files found by this linter.

Context for reviewers

PLEASE: double check the fixed links to ensure I found the right one. 🙂

This linter only checks broken links. There are other markdown linters we can consider adding should the need arise. This linter will be run when running the specific make command make infra-lint-markdown as well as the general lint command make infra-lint. Therefore, it will also be picked up as part of the CI pipeline which runs all the linters.

For the action, I largely copied the action used in GoogleChrome.

Testing

Before

Running locally before fixing the errors (snippet image)
image

And see the CI failing for just this new check before fixing the errors it finds:
image

Snippet of failing checks
image

After

Locally, make command completes without error when finding no issues.
image

All the beautiful checks in the CI for PRs pass! (Love those green checkmarks!)
image

@anybodys anybodys marked this pull request as ready for review December 7, 2023 22:35
@anybodys
Copy link
Contributor Author

anybodys commented Dec 7, 2023

(Not actually ready for review. Am trying to trigger CI pipeline to test that.)

@anybodys anybodys force-pushed the kmd/doc-link-linter branch 7 times, most recently from 8ee542a to a4f6b47 Compare December 7, 2023 23:25
@anybodys anybodys force-pushed the kmd/doc-link-linter branch 2 times, most recently from bf0c069 to 05f45dd Compare December 8, 2023 00:03
@anybodys anybodys force-pushed the kmd/doc-link-linter branch from 05f45dd to 7576c91 Compare December 8, 2023 00:05
@anybodys
Copy link
Contributor Author

anybodys commented Dec 8, 2023

(Not actually ready for review. Am trying to trigger CI pipeline to test that.)

Ready now! All checks are passing and docs fixed.

@anybodys anybodys requested a review from lorenyu December 8, 2023 00:07
Copy link
Contributor

@lorenyu lorenyu left a comment

Choose a reason for hiding this comment

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

Yay! Thanks

Makefile Outdated
infra-lint: infra-lint-markdown infra-lint-scripts infra-lint-terraform infra-lint-workflows ## Lint infra code

infra-lint-markdown: ## Lint Markdown docs for broken links
BASEURL=`pwd`; find . -name \*.md -print0 | xargs -0 -n1 markdown-link-check --config .github/workflows/markdownlint-config.json
Copy link
Contributor

Choose a reason for hiding this comment

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

I wish it was possible to add comments in Makefiles since I would have loved to have a comment explaining this line for shell noobs like myself.

Optional suggestion: Maybe move this line to ./bin/lint-markdown.sh where we can add comments and explain what it's doing so if anyone needs to update it they are better equipped

Suggested change
BASEURL=`pwd`; find . -name \*.md -print0 | xargs -0 -n1 markdown-link-check --config .github/workflows/markdownlint-config.json
./bin/lint-markdown.sh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great suggestion! It'll also make it easier to add other markdown lints later if we want. In the process of this change, I discovered BASEURL isn't as necessary as I thought. Must default to current working dir or something. Anyways, feel free to take another look and let me know if you have anything thoughts.

Copy link
Contributor

@lorenyu lorenyu left a comment

Choose a reason for hiding this comment

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

Meant to approve!

@anybodys anybodys force-pushed the kmd/doc-link-linter branch from 20089a6 to 38173dc Compare December 8, 2023 17:49
@anybodys anybodys merged commit fdb6e34 into main Dec 8, 2023
9 checks passed
@anybodys anybodys deleted the kmd/doc-link-linter branch December 8, 2023 18:08
@daphnegold
Copy link
Contributor

This is awesome

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken documentation link
3 participants