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

Propose mdox for docs formatting and link checking #4196

Merged
merged 3 commits into from
Oct 5, 2021

Conversation

saswatamcode
Copy link
Contributor

@saswatamcode saswatamcode commented Aug 2, 2021

Description

This PR proposes the use of mdox for formatting docs, both remote and local link checking, and for generating markdown code blocks within them.

  • Removes previous docs link check CI and replaces it with mdox which would fail on broken links or incorrect markdown formatting. Also, mdox can be run locally and shows diffs so commiter can check beforehand.
  • Removes embedmd and replaces it with mdox-exec code block directives which are much more customizable, as they use linux command outputs for formatting code blocks, for e.g yaml mdox-exec="cat example/storage/storageclass.yaml".
  • Adds make docs and make check-docs for running mdox fmt and mdox fmt --check

This check currently skips all generated docs, as specified in cmd/po-docgen.

Doc transforming can be added later!

Note: Would need feedback on how Makefile and CI use mdox and if it can be better! 🙂

cc: @bwplotka

Type of change

What type of changes does your code introduce to the Prometheus operator? Put an x in the box that apply.

  • CHANGE (fix or feature that would cause existing functionality to not work as expected)
  • FEATURE (non-breaking change which adds functionality)
  • BUGFIX (non-breaking change which fixes an issue)
  • ENHANCEMENT (non-breaking change which improves existing functionality)
  • NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

Changelog entry

Add mdox to CI for easier formatting, link checking, and generation of markdown code blocks in Documentation.

@saswatamcode saswatamcode requested review from paulfantom and a team as code owners August 2, 2021 11:24
CHANGELOG.md Outdated
`PrometheusRule` CRD. It addresses the need for rule syntax validation and rule
selection across namespaces. `PrometheusRule` replaces the configuration of
Prometheus rules via K8s ConfigMaps. There are two migration paths:
With this release we introduce a new Custom Resource Definition - the `PrometheusRule` CRD. It addresses the need for rule syntax validation and rule selection across namespaces. `PrometheusRule` replaces the configuration of Prometheus rules via K8s ConfigMaps. There are two migration paths:
Copy link
Member

Choose a reason for hiding this comment

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

Is there no line length limit in mdox? I don't see why we would like to switch from nicely formatted multiline text to single-line one. Especially since the latter one is harder to compare when using diff.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, is it really hareder to review?

Let us know if it's a blocker, we could add that feature, but it looks fine for me TBH.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

For me, yes, it is. With multi-line, GitHub would present just a small part that has changed, here it presents one giant block of changed text. I don't see why I would need to go through a 400 char paragraph to figure out the PR is just fixing a simple typo. Plus, if a paragraph is very long, GitHub will collapse file change in diff view.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @paulfantom! mdox now supports preserving soft line breaks(bwplotka/mdox#75). This PR has been updated with the same! 🙂

Copy link
Member

@paulfantom paulfantom left a comment

Choose a reason for hiding this comment

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

This is converting all nicely formatted multi-line paragraphs into single-line ones. Having text written this way will create usability issues when using diff (for example in PRs).

@bwplotka
Copy link
Contributor

bwplotka commented Aug 3, 2021

Yea, that's kind of a feature, you don't need multi-line in practice - all tools are wrapping lines for you, so why doing this manual work? 🤔 It's hard to edit anything.

In terms of diff, why there are usability issues? In terms of merging, you don't have correct merge options for human text anyway. 🤔

Anyway, we can add some option to wrap automatically to given length maybe, but to me there is no point ):

Copy link
Contributor

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Nice work, love it 👍🏽

It's up to all maintainers if they want this tool here, LGTM to me. Let's make sure everyone is happy.

CHANGELOG.md Outdated
`PrometheusRule` CRD. It addresses the need for rule syntax validation and rule
selection across namespaces. `PrometheusRule` replaces the configuration of
Prometheus rules via K8s ConfigMaps. There are two migration paths:
With this release we introduce a new Custom Resource Definition - the `PrometheusRule` CRD. It addresses the need for rule syntax validation and rule selection across namespaces. `PrometheusRule` replaces the configuration of Prometheus rules via K8s ConfigMaps. There are two migration paths:
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, is it really hareder to review?

Let us know if it's a blocker, we could add that feature, but it looks fine for me TBH.

image

@bwplotka
Copy link
Contributor

Looks like your point was addressed @paulfantom , do you think it's worth a try? (: cc @simonpasquier @fpetkovski

Copy link
Member

@paulfantom paulfantom left a comment

Choose a reason for hiding this comment

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

Sorry, I was on vacation for the last few weeks.

It looks good from my side and I would love to see this merged. Could you rebase it so merge conflicts are solved?

Thanks for all your help! 💪 👍

@saswatamcode
Copy link
Contributor Author

@paulfantom I've rebased and updated mdox to latest. 🙂

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
Copy link
Contributor

@philipgough philipgough left a comment

Choose a reason for hiding this comment

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

/lgtm

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.

None yet

4 participants