-
Notifications
You must be signed in to change notification settings - Fork 18
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
markdown linting added #122
Conversation
6da03e5
to
2cb847f
Compare
README.md
Outdated
@@ -411,7 +411,7 @@ tmp-vm01 1 de:ad:be:ef:00:01 192.168.105.100 | |||
|
|||
- Reload the DHCP daemon. | |||
|
|||
``` | |||
```bas |
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.
bash
.github/workflows/markdown.yaml
Outdated
- name: Run markdownlint with auto-fix | ||
uses: DavidAnson/markdownlint-cli2-action@v19 | ||
with: | ||
fix: true # Enable auto-fixing |
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 asked before why we enable fix: true. What is the purpose of this option?
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.
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.
And what is the output with fix: true
?
It seems that we should add a comment like this to make it more clear:
# Enabled to improve reported errors
fix: true
(The comment may be improved, I guess the output include the suggested fix)
The existing comment "Enable auto-fixing" is not useful because it explains what the option does which is pretty clear. Comments should explain why we do stuff, which something the code cannot explain.
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.
with fix:true
, these mistakes are not pointed out as errors and the check runs smoothly
I will update the comment
.github/workflows/markdown.yaml
Outdated
with: | ||
fix: true # Enable auto-fixing | ||
globs: '**/*.md' # Specify the files to lint | ||
config: '.markdownlint.json' # personal config file based over the choices told |
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 already commented on the comment: This is not a personal config file. Please remove the comment.
.github/workflows/markdown.yaml
Outdated
uses: DavidAnson/markdownlint-cli2-action@v19 | ||
with: | ||
fix: true # Enable auto-fixing | ||
globs: '**/*.md' # Specify the files to lint |
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 comment is not needed. Does it come from an example file?
2cb847f
to
88f7ef6
Compare
We already discussed this on another PR - please use the standard |
88f7ef6
to
e76ba8e
Compare
@nirs kindly review |
.github/workflows/build.yaml
Outdated
@@ -30,3 +30,9 @@ jobs: | |||
fetch-depth: 1 | |||
- name: Build | |||
run: make | |||
- name: Run markdownlint with auto-fix | |||
uses: DavidAnson/markdownlint-cli2-action@v19 |
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.
As an OSSF best practice we prefer to specify actions by SHA and not by version. You can look up the version like this:
❯ gh api repos/DavidAnson/markdownlint-cli2-action/git/ref/tags/v19 | jq -r .object.sha
05f32210e84442804257b2a6f20b273450ec8265
Dependebot will take care of updating from then on.
uses: DavidAnson/markdownlint-cli2-action@v19 | |
uses: DavidAnson/markdownlint-cli2-action@05f32210e84442804257b2a6f20b273450ec8265 # v19 |
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.
Thanks , I'll fix it
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.
@jandubois using digest looks good (I did not know that dependebot supports this) but do you have any references to this best practice?
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.
You can find it under pinned dependencies:
For GitHub workflows used in building and releasing your project, pin dependencies by hash. See main.yaml for example. To determine the permissions needed for your workflows, you may use StepSecurity's online tool by ticking the "Pin actions to a full length commit SHA". You may also tick the "Restrict permissions for GITHUB_TOKEN" to fix issues found by the Token-Permissions check.
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.
Dependebot will take care of updating from then on.
I just noticed that we have not set up dependabot for this repo yet; will create an issue for it.
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 have created a pullrequest for it as well :) and fixed this one too
e76ba8e
to
6d55e86
Compare
.github/workflows/build.yaml
Outdated
@@ -30,3 +30,10 @@ jobs: | |||
fetch-depth: 1 | |||
- name: Build | |||
run: make | |||
- name: Run markdownlint with auto-fix | |||
uses: DavidAnson/markdownlint-cli2-action@05f32210e84442804257b2a6f20b273450ec8265 |
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.
Note that I left the action version in a comment in my suggestion:
uses: DavidAnson/markdownlint-cli2-action@05f32210e84442804257b2a6f20b273450ec8265 | |
uses: DavidAnson/markdownlint-cli2-action@05f32210e84442804257b2a6f20b273450ec8265 # v19 |
When dependabot creates a PR to update the SHA, it will update the comment with the version too. I don't know if it will add a comment, if there isn't one, so I think it will be safer to add it right away.
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.
See for example https://github.com/lima-vm/lima/pull/3291/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.
got it , did'nt know that dependabot updates it as well
Signed-off-by: Kairvee Vaswani <[email protected]>
6d55e86
to
58960aa
Compare
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.
Thanks, LGTM
@@ -30,3 +30,10 @@ jobs: | |||
fetch-depth: 1 | |||
- name: Build | |||
run: make | |||
- name: Run markdownlint with auto-fix | |||
uses: DavidAnson/markdownlint-cli2-action@05f32210e84442804257b2a6f20b273450ec8265 #v19 |
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 hope this works; the examples I have seen all have a space after the #
. Unfortunately it isn't documented how exactly dependabot is updating comments.
Fixes #79
added the linting according to the rules discussed
in
json