ci: sync Markdownlint setup from mdn/content

[nschonni]

Copied the setup from the upstream repo, but added a step to copy over the package.json and yarn.lock to use the same version of the tools

[nschonni]

Note that some of the issues will be silenced with #7778, and normally the second Markdownlint that only runs on changed markdown files will be run.

/cc @caugner @yin1999 @OnkarRuikar

[OnkarRuikar]

We shouldn't link the linter version to the mdn/content project. Because there version gets updated after all the files are fixed.
Here also we need to make sure all the file are fixed before changing the linter version. The two projects are not linked in this regard.

---

We do not have to install all the content/yari dependancies to lint the translated content:

      - uses: actions/checkout@v3

      - name: Setup Node.js environment
        uses: actions/setup-node@v3
        with:
          node-version: "16"

      - name: Setup packages
        run: |
          npm init -y &> /dev/null
          # get the version from content project
          version=$( curl https://raw.githubusercontent.com/mdn/content/main/package.json 2>&1 | grep -oP "(?<=markdownlint-cli....)[\.\d]+" )
          npm install -g "markdownlint-cli@$version"

      - name: Lint markdown files
        run: |
          echo "::add-matcher::.github/workflows/markdownlint-problem-matcher.json"
          markdownlint "**/*.md"

[caugner]

We shouldn't link the linter version to the mdn/content project.

Very valid point from @OnkarRuikar, so how about having a workflow instead that opens a PR with these changes?

[nschonni]

We shouldn't link the linter version to the mdn/content project. Because there version gets updated after all the files are fixed.
Here also we need to make sure all the file are fixed before changing the linter version. The two projects are not linked in this regard.

They content is synced across, so I think I'm not sure why they wouldn't use the same tools. This repo hasn't been cleaned up and is still in the process of converting to Markdown, so having the task now would help flag stuff to cleanup as it lands.

If you think it shouldn't leverage the lock/package from the other project, I think there should be a separate copy in this repo+dependabot rather than embedded versions in the action files

[OnkarRuikar]

so I think I'm not sure why they wouldn't use the same tools.

It's not about the tools being same. It's about tool versions. The content project doesn't wait for this repo to update the tool versions.

If you think it shouldn't leverage the lock/package from the other project, I think there should be a separate copy in this repo+dependabot rather than embedded versions in the action files

I agree. We need to do npm init here and add only the required tools. And the dependabot.
To avoid accidental publish/release, set "private": true and avoid name and version props in the package.json. [ref].
For example, the mdast repo setup

[nschonni]

I ran npm init --y and cleaned it up a bit. Used Yarn to match the tooling upstream

[OnkarRuikar]

In order for contributors to avoid facing errors in local environment we need to provide engine version in package.json:
The markdownlint-cli supports engines >=14:

"engines": {
    "node": ">=14"
  },

We can have the same or as we are starting a fresh we can start with the latest engine >=18.0.0 (like yari)?

[nschonni]

I think CI is mostly 16 right now, so 14/16 would probably be better for now if you want to add the engine

[caugner]

I think CI is mostly 16 right now, so 14/16 would probably be better for now if you want to add the engine

Let's use the same version constraint as yari and content (after mdn/content#20240).

[nschonni]

I've add the engines to match the pending PR upstream, and added the MD051/52 disable to this PR (which knocks off 3K issues)

[nschonni]

@caugner I've got a bunch of PRs to cleanup a bunch of the issues, but this should be good to land

[caugner]

LGTM

[nschonni]

This can either land as-is, or I'll updated it to match mdn/content#20330 if that lands

[yin1999]

This can either land as-is, or I'll updated it to match mdn/content#20330 if that lands

I'm for updating to markdownlint-cli2.

[nschonni]

Flipping to draft to switch this over to CLI2, now that the upstream PR landed

[nschonni]

Ping @caugner it would be good to land so we can see if the autofix PRs work. It's OK to have the job failure right now

[nschonni]

@caugner do you want me to rebase/squash the commits?

[Rumyra]

Yes could you @nschonni - I'll see if we can get final review and merge 👍

[bsmth]

Also having a look 👀

[bsmth]

One comment open about using latest GH action, but can be addressed in a follow-up.

Edit: the gh action is now bumped to tj-actions/changed-files@v34

I don't see anything wrong (aside from failing CI 😄), so also leaving a LGTM 👍🏻

[nschonni]

The failures for the job can be used to test the Autofix PR job

[nschonni]

There is an odd fix for the headings in blockquote that I'm going to raise upstream, but not blocking this

[schalkneethling]

There is an odd fix for the headings in blockquote that I'm going to raise upstream, but not blocking this

Are we OK with Markdownlint failing? Looking at the log there are no "errors" but there are 20 warnings.

[nschonni]

Upstream, they changed it from Errors to Warning to the severity. I'm not sure I get the benefit, as the job will fail either way, but I guess it looks less scary on the reviewers.

The 20 issues are fine, as this PR includes a new job that will create PRs to fix the existing issues. I created a manual one for the Japanese texts since there are formatting issues that the autofix won't quite handle. Landing this is fine though

[OnkarRuikar]

Are we OK with Markdownlint failing? Looking at the log there are no "errors" but there are 20 warnings.

Yes. We don't want to burden online(on Github) contributors with lint errors, cos they may not know Markdown syntax, so the lint errors are marked as warning. Also because we have a bot in place that daily autofixes the lint issues.
We purposefully fail the job on warnings because it shows the indicator in PR list. And some maintainers proactively look for the failing test PRs and fix the lint issues.

Note: Fixing lint issues has been made mandatory for those who push changes using git.

[github-actions]

This pull request has merge conflicts that must be resolved before it can be merged.

[nschonni]

Thanks @caugner ! Now to wait and see how the auto-PRs go 😄

[yin1999]

Sorry for ignoring the PAT field, should we use AUTOMERGE_TOKEN in the markdownlint-fix workflow (for triggering PR test)?

translated-content/.github/workflows/sync-translated-content.yml

Line 67 in e0c9430

token: ${{ secrets.AUTOMERGE_TOKEN }}

[nschonni]

Yeah, I guess that would be needed to have the PRs trigger subsequent CI jobs

[OnkarRuikar]

@hochan222 The error is not related to the changes done in that PR; it's related to the workflow code. Try re-running the workflow. Go to the latest failing check's log and click on Re-run jobs -> Re-run all jobs.

If that fails rebase the PR's branch, which will automatically trigger all the workflows..

---

Linting is should not mandatory. I suggest simply merge the PR. If there are linting errors, they will be fixed by cron job next midnight UTC.

[hochan222]

@OnkarRuikar Thank you for your reply. The error I encountered in various PRs was that the markdownlint workflow failed after further changes, even though the first time passed fine. Even running the workflow again failed the same. In the end, I asked contributors to rebase or re-commit for this. (Even though there are actually no markdownlint errors(maybe..?).. just for workflow)

Also, In the way to merge right away... the reason I wrote it for confirmation was because I was concerned about that it was displayed as an X ​​rather than a check. I'm afraid it will set a bad precedent. Nevertheless, if it's okay, I think it's better to just merge.

[hochan222]

I merged PRs that failed the workflow, but the check went well as shown below. I think just merging would be fine.