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

Add Markdown diff linter #879

Merged
merged 1 commit into from
Aug 8, 2024
Merged

Conversation

ivanvc
Copy link
Member

@ivanvc ivanvc commented Aug 6, 2024

I had this issue open for a while but couldn't get back to it. It adds a GitHub workflow (EDIT: we decided to make this a standalone script, so it can be used with prow) that links changed Markdown files and fails if the issue is within the changed lines.

It uses NodeJS' markdownlint-cli2, which has improvements over markdownlint-cli. I introduced a configuration file, .markdownlint-cli2.yaml, which allows lines longer than 80 characters, as this rule would raise issues in every Markdown file we have in the repository.

This should help with reviews, avoiding discussing Markdown formatting.

An example of a run at my fork: ivanvc#6.

Fixes #857.

@ivanvc
Copy link
Member Author

ivanvc commented Aug 6, 2024

/hold

@ivanvc ivanvc force-pushed the add-markdown-diff-linter branch from 3864c40 to 89feba7 Compare August 6, 2024 23:09
@ivanvc
Copy link
Member Author

ivanvc commented Aug 6, 2024

/unhold
cc. @jmhbnz

Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

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

Many thanks for getting this underway @ivanvc. Question below about pipeline approach, want to cover that off first before we go to much further.

.github/workflows/markdown-diff-lint.yaml Outdated Show resolved Hide resolved
@ivanvc ivanvc force-pushed the add-markdown-diff-linter branch 2 times, most recently from 2d222a1 to cffd6de Compare August 7, 2024 23:48
@ivanvc ivanvc requested a review from jmhbnz August 7, 2024 23:49
Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

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

LGTM - The make markdown-diff-lint recipe works well for me locally.

One minor nit about dependency install below, thanks @ivanvc

scripts/markdown_diff_lint.sh Outdated Show resolved Hide resolved
Add Makefile target, and scripts to lint only the modified Markdown
files, failing only if the violation is within the changed lines.

Signed-off-by: Ivan Valdes <[email protected]>
@ivanvc ivanvc force-pushed the add-markdown-diff-linter branch from cffd6de to 954cf31 Compare August 8, 2024 18:20
@jmhbnz jmhbnz requested a review from spzala August 8, 2024 18:35
Copy link
Member

@spzala spzala left a comment

Choose a reason for hiding this comment

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

Great work @ivanvc Thank you!

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ivanvc, jmhbnz, spzala

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

@jmhbnz jmhbnz merged commit 920a9f0 into etcd-io:main Aug 8, 2024
5 checks passed
@ivanvc ivanvc deleted the add-markdown-diff-linter branch August 22, 2024 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable Markdown linting
4 participants