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: small format changes in markdown files #26

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

Conversation

yriveiro
Copy link

@yriveiro yriveiro commented Dec 18, 2023

Description

This PR performs formatting changes on top of some markdown files to improve standardization.

As part of this PR two new workflows were added:

  • One to linting markdown files using markdownlint as added as well as suggested in the comments
  • A problem-matcher workflow

The sets of rules are the default ones defined in markdownlint rules, if the community decide to drift from the current default, markdownlint will look .markdownlint-cli2.jsonc for custom configurations.

@yriveiro
Copy link
Author

Do I need to add the changelog/PR.txt file as well? From what I saw the PR counter was reset in Openbao and the ones in the folder are 24XXX (I assuming, they are PR number from the backports)

@naphelps
Copy link
Member

@yriveiro The community has not formally decided what the changelog format and structure will be under Openbao. I am not sure documentation adjustments warrant a standalone line item in the changelog.

@naphelps naphelps self-requested a review December 22, 2023 01:59
@cipherboy
Copy link
Contributor

cipherboy commented Dec 22, 2023

I am not sure documentation adjustments warrant a standalone line item in the changelog.

I concur. I've filed #31 for discussing the changelog process in general. :-)

If we are going to merge this, and IMO, I'm not sure I have strong feelings either way, could we add another gating test for markdown files to comply?

@yriveiro
Copy link
Author

Changes in markdown files should not trigger go linter, at the end of the day, you are consuming action minutes in files that are not in the scope.

@cipherboy
Copy link
Contributor

@yriveiro Sorry, let me rephrase. If we're going to accept a PR doing markdown linting, we should add a markdown linting workflow so all future PRs will comply, rather than periodically drifting and re-syncing with the linter as someone runs it manually. :-) Otherwise, there's no value in accepting this PR, as it will just continue to drift as not everyone naturally writes markdown that compiles with this particular linter.

In general, there's probably an argument to be said about compatibility and preventing obvious non-markdown, so I think there's probably value in adding the linter to the workflows. My 2c!

@yriveiro
Copy link
Author

😄 OK got it, I will work in the workflow linting as part of this PR.

branches:
- main
paths:
- "**/*.md"
Copy link
Contributor

Choose a reason for hiding this comment

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

@yriveiro Not to cause more questions for you... But do you know if it supports .mdx files? All of our API/usage docs are currently using a combination of Markdown and JSX used by Vercel, that could be linted as well. :-)

Copy link
Author

@yriveiro yriveiro Dec 28, 2023

Choose a reason for hiding this comment

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

Unfortunately, markdownlint doesn't support mdx files DavidAnson/markdownlint#723 (comment)

One way to overcome this could be adding a new step to the workflow that renders mdx files with something like @mdx-js/mdx. Once we get the output in a temp directory and run the linter, not clear to me how we will find the line on the source code that triggers the linter as we are working on top of the rendered code.

Other options could be using remark-lint.

The first one is straightforward, but if we have the requirement of support mdx files, the second will probably fit better, never used remark-lint. It will take more time, but is doable 😄

Thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

It seems that what I said in the previous comment is not accurate, indeed markdownlint doesn't support mdx files, nevertheless, the action uses markdownlint-cli2 and this one supports mdx.

The folder website will probably throw a lot of warnings as most of the code is generated and that implies that the generator needs to implement the defined rules the community decides first.

Copy link
Contributor

@cipherboy cipherboy Dec 30, 2023

Choose a reason for hiding this comment

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

Ah cool!

From what I recall, I don't think many of the .mdx files under website/ are auto-generated. I know a lot of them were manually maintained, so I don't think we're liable to overwrite them any time soon. That said, I definitely agree, there's a lot of stuff there that is not well formatted and it would be time consuming to fix... (unlike an auto-generated version, where we could update the generator to be more compliant).

It doesn't look like there's an easy changed-files-only version of this workflow... I think we'd have to look at GITHUB_BASE_REF and GITHUB_SHA to see which files changed and then look for **/*.mdx files (like we do for the changelog checker). Perhaps best to ignore them for now then given there are 1k+ of them...

Sorry for the dead-end! But let's keep this in mind for later as we start thinking about what to do with the website docs. :-)

@yriveiro yriveiro marked this pull request as draft December 28, 2023 10:35
cipherboy pushed a commit to cipherboy/openbao that referenced this pull request Jan 21, 2024
cipherboy pushed a commit to cipherboy/openbao that referenced this pull request Jan 21, 2024
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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

3 participants