(actions) Add markdown lint job#32
Conversation
Tidy was being instructed to `go mod tidy` the hack/tools folder, which was being done when the tools were being downloaded using go modules (the main branch has the setup to download the tools using go installer instead)
Also update the README to incorporate changes to the project since bootstrapping.
|
Hold reviews for #29 to go in first. |
| uses: actions/checkout@v3 | ||
|
|
||
| - name: Lint markdown files | ||
| uses: github/super-linter/slim@v4 |
There was a problem hiding this comment.
(repeating question from original PR and adding another:
- Is this something that could easily be run locally (ala our other build tools that we pull/run under make targets)? If so, I'd prefer we follow that pattern to make our CI environment reproducible locally. (based on this, I don't see us being cool with npm and node as a local build tool dependency)
- What are we trying to catch with this? It's hard to tell from the markdownlint readme what value it brings. Reason I ask is because SDK's markdown linting is flaky (due to link checking) and is sometimes helpful but sometimes actively in the way.
There was a problem hiding this comment.
Is this something that could easily be run locally (ala our other build tools that we pull/run under make targets)? If so, I'd prefer we follow that pattern to make our CI environment reproducible locally. (based on this, I don't see us being cool with npm and node as a local build tool dependency)
+1 on not adding npm+node as a dependency. If we felt we needed this we could instead use the docker image like:
$(CONTAINER_TOOL) run -v $PWD:/workdir ghcr.io/igorshubovych/markdownlint-cli:latest "*.md"
where CONTAINER_TOOL is another Makefile variable. This should be fine since we are already requiring Docker usage in the Makefile at the moment. As an aside, it might be nice, as another follow up, to allow the use of Podman+Docker in our Makefile (not pressing but just something to think about).
What are we trying to catch with this? It's hard to tell from the markdownlint readme what value it brings. Reason I ask is because SDK's markdown linting is flaky (due to link checking) and is sometimes helpful but sometimes actively in the way.
From what I can tell rukpak enabled it to ensure the documentation is more uniform by running the linter against it. I personally think that might be a bit overkill for us right now especially because we have no catalogd documentation at the moment. I think if we notice that consistent docs formatting is causing us major headache down the road then it might be worth adding this, but until then I would vote for leaving this out.
There was a problem hiding this comment.
it feels right to have at least some guard rails around documents we add though. In OLM we had a different experience without any guard rails being preset. It's very easy for the docs to get out of date, and at the minimum if a PR author is made aware that the changes being introduced in the PR has broken a link mentioned in a doc, it forces the PR author to also update the doc with the PR.
The other fixes I had to do for this PR for example was all related to healthy doc writing habits (space between sections etc) that make docs look readable so I'm +1 for them.
(Holding off on any changes to this PR until we're in agreement about adding/not adding this check. +1 on making sure the binary is downloadable and runnable using a make target so that PR authors can run it locally, I can look into that if we decide to move ahead with this).
There was a problem hiding this comment.
Using a docker image is definitely preferable to local npm+node. But it also does feel a little dirty, tbh. @anik120 Do you know if there are any alternative markdown linters written in Go that we could use/follow the same pattern as our other tools? That would be ideal.
Re: what does this get us, I think I saw:
- link checking
- some sort of "best practice" opinions on markdown formatting.
Not necessarily opposed, but it would be nice to enumerate what we're hoping to get out of this so that we aren't constantly chasing some linter rules over time that we don't actually care about.
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
4 participants
No description provided.