-
Notifications
You must be signed in to change notification settings - Fork 454
fix: Use GitHub Actions for markdown linting in ci-docs
#718
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
Conversation
| @@ -1,5 +1,4 @@ | |||
| { | |||
| "globs": ["website/**/*.md"], | |||
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 globs field in .markdownlint-cli2.jsonc was removed because it was overriding the actual action glob input and causing the linter to check all markdown files in the repository instead of only the files that were changed in the PR.
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.
I'd like to confirm if your intention is that this CI will only check the changed files?
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.
Yes, that's right. The CI will only check markdown files that were changed in the PR as I found that the globs field in the config file was merging with the action input. When I tested with just website/test-markdown.md as the input here, the linter actually found and checked 102 files instead of just that one file. The log showed it was linting both my input file and all files matching website/**/*.md from the config. Removing the globs field fixes this so only changed files get linted, which means existing lint errors in other files won't block new PRs. What do you think about this approach?
Note: Currently there are 12 actual linting errors in the markdown files in the repository.
Error details
Finding: website/test-markdown.md website/**/*.md !**/node_modules/** !**/target/** !**/dist/**
Linting: 102 file(s)
Summary: 15 error(s)
Error: website/docs/migration/from-fastexcel.md:47 MD031/blanks-around-fences Fenced code blocks should be surrounded by blank lines [Context: "```xml"] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md031.md
Error: website/docs/migration/from-fastexcel.md:56 MD031/blanks-around-fences Fenced code blocks should be surrounded by blank lines [Context: "```gradle"] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md031.md
Error: website/docs/migration/from-fastexcel.md:104 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- Update Maven/Gradle dependen..."] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md032.md
Error: website/docs/migration/from-fastexcel.md:110 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- Progressively replace deprec..."] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md032.md
Error: website/docs/migration/from-fastexcel.md:116 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- Remove all references to dep..."] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md032.md
Error: website/docs/migration/from-fastexcel.md:121 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- Lower risk through increment..."] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md032.md
Error: website/i18n/zh-cn/docusaurus-plugin-content-docs/current/migration/from-fastexcel.md:47 MD031/blanks-around-fences Fenced code blocks should be surrounded by blank lines [Context: "```xml"] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md031.md
Error: website/i18n/zh-cn/docusaurus-plugin-content-docs/current/migration/from-fastexcel.md:56 MD031/blanks-around-fences Fenced code blocks should be surrounded by blank lines [Context: "```gradle"] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md031.md
Error: website/i18n/zh-cn/docusaurus-plugin-content-docs/current/migration/from-fastexcel.md:104 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- 将 Maven/Gradle 依赖更新为 Apache ..."] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md032.md
Error: website/i18n/zh-cn/docusaurus-plugin-content-docs/current/migration/from-fastexcel.md:110 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- 逐步将已废弃的类替换为 FesodSheet"] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md032.md
Error: website/i18n/zh-cn/docusaurus-plugin-content-docs/current/migration/from-fastexcel.md:116 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- 删除所有对已废弃类的引用"] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md032.md
Error: website/i18n/zh-cn/docusaurus-plugin-content-docs/current/migration/from-fastexcel.md:121 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- 通过增量更改降低风险"] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md032.md
Error: website/test-markdown.md:1 MD041/first-line-heading/first-line-h1 First line in a file should be a top-level heading [Context: "### This is h3 without h1 firs..."] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md041.md
Error: website/test-markdown.md:5 MD040/fenced-code-language Fenced code blocks should have a language specified [Context: "```"] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md040.md
Error: website/test-markdown.md:9:38 MD047/single-trailing-newline Files should end with a single newline character https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md047.md
Error: Failed with exit code: 1There 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.
This CI needs to check all the markdown documents, not just the files related to this PR. This ensures that no issues such as omissions or incorrect merges will be overlooked.
This might result in the detection of document ranges that were not modified by this PR, but this is not a problem. It can also help us improve the documentation.
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.
I see, thanks for clarifying. I'll work on updating the workflow to check all markdown files.
website/test-markdown.md
Outdated
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.
A test markdown file with intentional violations is included to demonstrate the workflow catches issues correctly. This file will be removed after the CI run fails to prove the linting works.
|
Hi apologies @delei, I've updated the workflow to add the globs parameter to the lint step and remove the test markdown file. Could you help trigger the CI again to verify it works correctly? You can verify the log from my repo since we don't change anything to the |
| on: | ||
| pull_request: | ||
| paths: | ||
| - 'website/**' |
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.
Hi @delei, currently ci-docs only triggers on website/** changes. Should we also add .github/workflows/ci-docs.yml to the paths so the workflow runs when we modify it? I feel it's better safe than sorry as when someone changes this workflow file, we might want to test the ci-docs workflow again to make sure it still works correctly. Just wondering what you think about this?
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.
Typically, we do not include modifications within the .github directory as part of CI process.
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.
Got it, will keep it as current version. Thanks for clarifying!
delei
left a comment
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.
LGTM
Related: #703 (comment)
Purpose of the pull request
The markdown linting step in
ci-docswasn't working properly where the bash script had git reference errors that caused it to skip linting.What's changed?
This PR replaces the bash script with GitHub Actions for markdown linting. It uses:
Checklist