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

make lint-md failing for v16.x-staging in GitHub actions #39850

Closed
targos opened this issue Aug 23, 2021 · 7 comments
Closed

make lint-md failing for v16.x-staging in GitHub actions #39850

targos opened this issue Aug 23, 2021 · 7 comments
Labels
meta Issues and PRs related to the general management of the project.

Comments

@targos
Copy link
Member

targos commented Aug 23, 2021

See https://github.com/nodejs/node/runs/3399539405

I don't understand what the error is about and I cannot reproduce it locally.

@targos targos added meta Issues and PRs related to the general management of the project. v16.x labels Aug 23, 2021
@targos
Copy link
Member Author

targos commented Aug 23, 2021

@Trott maybe it's related to the recent markdown linter updates?

@aduh95
Copy link
Contributor

aduh95 commented Aug 23, 2021

lint-md should skip the check for release version number in changelog on branches that are not master, so there something wrong with this check:

- name: Get release version numbers
if: ${{ github.event.pull_request.base.ref == github.event.pull_request.base.repo.default_branch }}
id: get-released-versions
run: ./tools/node-lint-md-cli-rollup/src/list-released-versions-from-changelogs.mjs

To reproduce locally, you would need to pass the list of released version numbers as an environment variable; it would be worth adding a make command to do it easily, but currently you need to copy/paste the list manually (or come up with a clever sed command that does it for you):

$ tools/node-lint-md-cli-rollup/src/list-released-versions-from-changelogs.mjs
::set-output name=NODE_RELEASED_VERSIONS::9.11.2,...,0.12.0
$ NODE_RELEASED_VERSIONS="9.11.2,...,0.12.0" make lint-md

@Trott
Copy link
Member

Trott commented Aug 23, 2021

It's complaining about v14.17.0 in this YAML block in doc/api/http.md:

<!-- YAML
added: v0.3.6
changes:
  - version: v16.7.0
    pr-url: https://github.com/nodejs/node/pull/39310
    description: When using a `URL` object parsed username and
                 password will now be properly URI decoded.
  - version:
      - v15.3.0
      - v14.17.0
    pr-url: https://github.com/nodejs/node/pull/36048
    description: It is possible to abort a request with an AbortSignal.
  - version:
     - v13.8.0
     - v12.15.0
     - v10.19.0
    pr-url: https://github.com/nodejs/node/pull/31448
    description: The `insecureHTTPParser` option is supported now.
  - version: v13.3.0
    pr-url: https://github.com/nodejs/node/pull/30570
    description: The `maxHeaderSize` option is supported now.
  - version: v10.9.0
    pr-url: https://github.com/nodejs/node/pull/21616
    description: The `url` parameter can now be passed along with a separate
                 `options` object.
  - version: v7.5.0
    pr-url: https://github.com/nodejs/node/pull/10638
    description: The `options` parameter can be a WHATWG `URL` object.
-->

@Trott
Copy link
Member

Trott commented Aug 23, 2021

I think cherry-picking 16e00a1 will fix the lint issue but I don't know if it will land cleanly and I don't know if cherry-picking those kinds of commits is part of the usual release process.

@targos
Copy link
Member Author

targos commented Aug 23, 2021

We don't cherry-pick release commits between release branches.
I think @aduh95 is right and the problem is that this check is done when it's not supposed to be.

@Trott
Copy link
Member

Trott commented Aug 23, 2021

We don't cherry-pick release commits between release branches.
I think @aduh95 is right and the problem is that this check is done when it's not supposed to be.

Yeah, I'd be for not doing that check on staging/release branches etc.

@targos
Copy link
Member Author

targos commented Sep 7, 2021

I suspect that the condition github.event.pull_request.base.ref == github.event.pull_request.base.repo.default_branch previously evaluated to false for non-pull request events but GitHub changed something and now it evaluates to true.

targos added a commit to targos/node that referenced this issue Sep 7, 2021
BethGriggs pushed a commit that referenced this issue Sep 21, 2021
Fixes: #39850

PR-URL: #40027
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
BethGriggs pushed a commit that referenced this issue Sep 21, 2021
Fixes: #39850

PR-URL: #40027
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants