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

chore(lint): Fixed 90-95% of Markdown lint violations across the repository #12587

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Brijeshthummar02
Copy link

What I did

Related issue

(not mandatory) A picture of a cute animal, if possible in relation to what you did

@Brijeshthummar02 Brijeshthummar02 requested a review from a team as a code owner February 26, 2025 13:19
@glours
Copy link
Contributor

glours commented Feb 26, 2025

All the markdown files in the doc/reference are generated, your PR won't pass the CI

@Brijeshthummar02
Copy link
Author

@glours i just fixed lint issue of markdown i don't think it will affect the test case.

@ndeloof
Copy link
Contributor

ndeloof commented Feb 26, 2025

@Brijeshthummar02 this is not about test cases: files you edited are generated from equivalent yaml files and analysis of go code. CI validates they are up-to-date. By editing those generated files and not the actual source of truth you won't pass the CI

@ndeloof
Copy link
Contributor

ndeloof commented Feb 26, 2025

Also, if you want to promote some Markdown linter, please add a CI workflow to let it run on PRs, not just as a one-time fix

Copy link
Contributor

@ndeloof ndeloof left a comment

Choose a reason for hiding this comment

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

need to apply change on reference files, not generated markdown

@Brijeshthummar02
Copy link
Author

@ndeloof can u suggest exactly which reference files or you mean by all reference files.

@Brijeshthummar02
Copy link
Author

On note i am trying to add GitHub Actions workflow using markdownlint-cli.

@ndeloof
Copy link
Contributor

ndeloof commented Feb 26, 2025

see https://github.com/docker/compose/blob/main/docs/reference/docker_compose.yaml

On note i am trying to add GitHub Actions workflow using markdownlint-cli.

:+1 please add a CI workflow in your PR as well then

@glours
Copy link
Contributor

glours commented Feb 26, 2025

I would prefer to fix the root cause instead of having a GHA commiting code on user's behalf.
In addition as we're using the same doc generator for all the Docker commands line and I'm afraid the Compose reference doc won't display the same way in docs.docker.com.
IMHO it's better to propose this as part of github.com/docker/cli-docs-tool to be sure the YAML is right at generation time

@Brijeshthummar02
Copy link
Author

@glours @ndeloof
Would you prefer I open an issue or PR in docker/cli-docs-tool to ensure the YAML is correct at generation time?
Does this mean the current PR won't be merged, or should I adjust it?

@Brijeshthummar02
Copy link
Author

@ndeloof as requested added workflows to this PR and fixed it by not making changes auto instead it shows error in test and if no markdown file changes detected it will simply skip it also made it not to install any other or linters if no changes found.

@ndeloof
Copy link
Contributor

ndeloof commented Feb 26, 2025

The current PR could be merged as long as you don't just fix markdown, but also fix the content source, so that running make docs would produce the same markdown output.

A possible blocker is about docker/docs being populated based on same reference files (see https://github.com/docker/compose/blob/main/.github/workflows/docs-upstream.yml) - we need to be sure those change won't break docs.

Last but not least, I'm not sure we have huge interest adopting a markdown linter, as this doesn't seem to have any impact on our docs nor on existing markdown files in this repo, so maybe better give up with this

@Brijeshthummar02
Copy link
Author

On to that [Last but not least, I'm not sure we have huge interest adopting a markdown linter, as this doesn't seem to have any impact on our docs nor on existing markdown files in this repo, so maybe better give up with this] it just make files clean nothing much and initially this PR was just for fixing existing violations but you said to add a CI workflow to let it run on PRs, not just as a one-time fix. So what should we do now?

@ndeloof
Copy link
Contributor

ndeloof commented Feb 26, 2025

fixing existing violations

Those seems to be minor, and not aligned with docker/docs conventions as reported by @glours regarding console examples

but you said to add a CI workflow to let it run on PRs

yes, fixing linter violation only make sense if the linter is also part of your PR, otherwise this is only a question of personal preferences.

So what should we do now?

Maybe you can tweak the linter rules so it requires less changes and has limited impact on our docs ?
Otherwise, we will just close this PR, as this brings minor benefits to the project

@ndeloof
Copy link
Contributor

ndeloof commented Feb 26, 2025

after your last update, changes still only apply to generated markdown files, not to the actual source of truth (yaml files in same folder).

Signed-off-by: Brijeshthummar02 <[email protected]>

:wq

Signed-off-by: Brijeshthummar02 <[email protected]>

:wq
@glours
Copy link
Contributor

glours commented Feb 26, 2025

@Brijeshthummar02 I started the CI workflow to show you what will happen if the fix aren't done at the right place
https://github.com/docker/compose/actions/runs/13547678326/job/37866437567?pr=12587

As you can see we have a validation step in which the CI is checking that the documentation is up-to-date (flag added/removed, new description of a command, new command added ...).
This step is done by running make validate-docs and this Makefile command will use the docs-validate step of the Dockerfile and if you continue like this you will see that we generate the documentation and then make a git diff to check if there is differences.

So doing manual changes won't work there, the validation will fail each time and because all the changes in the docs/reference directory you proposed won't match the doc gen outputs
So you can edit only markdown section which are out of the <!---MARKER_GEN_START--> and <!---MARKER_GEN_END--> tags

@Brijeshthummar02
Copy link
Author

@glours
On running the validation command make validate-docs
it showed this.

Screenshot 2025-02-27 165415

Screenshot 2025-02-27 165430

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.

4 participants