tools: add GitHub Action linter for pr-url #37221

aduh95 · 2021-02-04T12:07:45Z

The PR URL is unknown when someone starts working on a change, they may
be tempted to use a placeholder with a "fake" PR URL value to make the
linter pass on their local machine. This commit adds warnings when a
pr-url doesn't correspond to the actual PR URL to help reviewers spot
those "fake" URLs before landing.

richardlau

Will this result in false positives for the release PR's (e.g. see the doc changes in #36476)?

Also I wonder if it's possible to detect/pass the expected pr-url into the existing YAML linting done in https://github.com/nodejs/remark-preset-lint-node/blob/0b2a13018de6ad9bcd234e4420bba03ae9f42c29/remark-lint-nodejs-yaml-comments.js#L134-L139 (see https://github.com/nodejs/node/runs/1825250022#step:5:11 for an example of a detected malformed pr-url)

aduh95 · 2021-02-04T15:15:19Z

Will this result in false positives for the release PR's (e.g. see the doc changes in #36476)?

Yes, it would print out warnings, but the workflow would still end with a green ticks – it only outputs warnings and not errors (so it wouldn't interfere with ncu or the commit queue).
IMHO it's worth it, it's easy to miss this with human eyes and we end up with PR like #37205 from time to time.

Also I wonder if it's possible to detect/pass the expected pr-url into the existing YAML linting

I remember trying to do that when I added the YAML linting a while back, but couldn't find a way: the linter parses all .md files of the code base, and doesn't know which lines are being add in a specific PR (nor the PR URL, but that could be set up in an environment variable).

RaisinTen · 2021-02-04T15:29:34Z

tools/lint-pr-url.mjs

+import process from 'node:process';
+import readline from 'node:readline';


Suggested change

import process from 'node:process';

import readline from 'node:readline';

import readline from 'readline';

nit: since process is a part of the global object
question: why are we prepending 'node:' to the imported module names?

nit: since process is a part of the global object

I try to not rely on Node.js-only globals when writing ES modules to ease a potential port to the browser. I really don't feel strongly about that, and am willing to remove the import if anyone disagrees.

question: why are we prepending 'node:' to the imported module names?

Every time we try to introduce a new module in core comes the question of how breaking it would be for the ecosystem (is there a module that use the same name out there). It's been discussed several time to add a prefix to core modules, and with ESM we had to come up with a URL scheme to be spec compliant. TSC has decided that it will be ported to the CJS resolver as well (see #36098), so why not use it.
Again, I don't feel strongly about that either, if the consensus is not to use the prefix, that'd be fine with me.

RaisinTen

The currentLine gets incremented over parts of the diff log where
line is deleted (prepended with -s) whereas we want currentLine
to remain the same while traversing deletions. Hence, the warning will
point to incorrect line numbers though line holds the correct value.
How should we fix this?

tools/lint-pr-url.mjs

RaisinTen · 2021-02-08T12:17:51Z

tools/lint-pr-url.mjs

+const changeDelimiter = /^@@ -\d+,\d+ \+(\d+),\d+ @@/;
+const prUrlDefinition = /^\+\s+pr-url: (.+)$/;
+
+const validatePrUrl = (url) => url == null || url === expectedPrUrl;


Suggested change

const validatePrUrl = (url) => url == null || url === expectedPrUrl;

const validatePrUrl = (url) => url === expectedPrUrl;

RaisinTen · 2021-02-08T12:18:26Z

tools/lint-pr-url.mjs

+
+let currentFile;
+let currentLine;
+


Suggested change

let currentPrUrl;

RaisinTen · 2021-02-08T12:19:48Z

tools/lint-pr-url.mjs

+    console.log(`Parsing changes in ${currentFile}.`);
+  } else if (changeDelimiter.test(line)) {
+    currentLine = Number(line.match(changeDelimiter)[1]);
+  } else if (!validatePrUrl(line.match(prUrlDefinition)?.[1])) {


Suggested change

} else if (!validatePrUrl(line.match(prUrlDefinition)?.[1])) {

} else if (currentPrUrl = line.match(prUrlDefinition)?.[1] &&

validatePrUrl(currentPrUrl)) {

RaisinTen · 2021-02-08T12:25:01Z

@aduh95 currently, validatePrUrl returns true for null, so, no warning is thrown for lines like:

+ pr-url:

Added some suggestions to fix that.

RaisinTen · 2021-02-08T12:26:45Z

.github/workflows/linters.yml

@@ -98,3 +98,12 @@ jobs:
      - uses: mszostok/[email protected]
        with:
          checks: "files,duppatterns"
+  lint-pr-url:
+    if: ${{ github.event.pull_request }}


Suggested change

if: ${{ github.event.pull_request }}

if: github.event.pull_request.draft == false

Isn't this supposed to be the check here (based on the above jobs)?

I meant to check only pull request (it wouldn't work on push, because it needs a PR URL), and I believe it's fine to have it running on draft PRs because it doesn't produce errors.

aduh95 · 2021-02-08T13:16:24Z

@aduh95 currently, validatePrUrl returns true for null, so, no warning is thrown for lines like:
+ pr-url: 
Added some suggestions to fix that.

That doesn't seem necessary, that kind of checks are already made by the linter, it would produce a warning on top of the error from lint-md.

RaisinTen · 2021-02-08T13:56:32Z

Can we disable lint-md from doing such checks?
I was considering adding this change here because lint-md only runs on PRs
that are ready for review, so pr-urls wouldn't be linted completely by this tool:

node/.github/workflows/linters.yml

Lines 42 to 43 in d75f3e9

    
           lint-md: 
        
             if: github.event.pull_request.draft == false

aduh95 · 2021-02-08T15:25:30Z

Can we disable lint-md from doing such checks?
I was considering adding this change here because lint-md only runs on PRs
that are ready for review, so pr-urls wouldn't be linted completely by this tool:

If you open a PR to re-enable linter GH actions for draft PR, I'd gladly approve it (I'm with with on that, I think it makes opening draft PR useless). I don't think it's a good idea to make lint-md more permissive, I've added this rule because there were quite a few mistakes that was spotted only when the rule was added; removing it would almost certainly mean we'll start seeing a few of those mistakes hoping here and there.

nodejs-github-bot · 2021-02-08T16:32:39Z

CI: https://ci.nodejs.org/job/node-test-pull-request/36016/

nodejs-github-bot · 2021-02-08T23:33:34Z

CI: https://ci.nodejs.org/job/node-test-pull-request/36024/

nodejs-github-bot · 2021-02-09T08:48:55Z

CI: https://ci.nodejs.org/job/node-test-pull-request/36028/

PR-URL: nodejs#37221 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Rich Trott <[email protected]>

aduh95 · 2021-02-09T10:46:04Z

Landed in 33d3a2e

PR-URL: #37221 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Rich Trott <[email protected]>

nodejs-github-bot added meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. labels Feb 4, 2021

aduh95 added the wip Issues and PRs that are still a work in progress. label Feb 4, 2021

aduh95 marked this pull request as draft February 4, 2021 13:08

aduh95 force-pushed the pr-url-lint branch from 918b923 to db0d74c Compare February 4, 2021 14:38

aduh95 marked this pull request as ready for review February 4, 2021 14:40

aduh95 closed this Feb 4, 2021

aduh95 reopened this Feb 4, 2021

aduh95 removed the wip Issues and PRs that are still a work in progress. label Feb 4, 2021

richardlau reviewed Feb 4, 2021

View reviewed changes

richardlau approved these changes Feb 4, 2021

View reviewed changes

RaisinTen reviewed Feb 4, 2021

View reviewed changes

RaisinTen suggested changes Feb 5, 2021

View reviewed changes

RaisinTen reviewed Feb 6, 2021

View reviewed changes

tools/lint-pr-url.mjs Show resolved Hide resolved

RaisinTen reviewed Feb 6, 2021

View reviewed changes

tools/lint-pr-url.mjs Outdated Show resolved Hide resolved

aduh95 requested a review from RaisinTen February 8, 2021 10:34

RaisinTen approved these changes Feb 8, 2021

View reviewed changes

RaisinTen reviewed Feb 8, 2021

View reviewed changes

tools/lint-pr-url.mjs

let currentFile;

let currentLine;

Copy link

Contributor

RaisinTen Feb 8, 2021

Choose a reason for hiding this comment

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

Suggested change

let currentPrUrl;

RaisinTen reviewed Feb 8, 2021

View reviewed changes

RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 8, 2021

Trott approved these changes Feb 9, 2021

View reviewed changes

tools: add GitHub Action linter for pr-url

33d3a2e

PR-URL: nodejs#37221 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Rich Trott <[email protected]>

aduh95 force-pushed the pr-url-lint branch from d75f3e9 to 33d3a2e Compare February 9, 2021 10:45

aduh95 merged commit 33d3a2e into nodejs:master Feb 9, 2021

aduh95 deleted the pr-url-lint branch February 9, 2021 10:46

danielleadams pushed a commit that referenced this pull request Feb 16, 2021

tools: add GitHub Action linter for pr-url

6db5e79

PR-URL: #37221 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Rich Trott <[email protected]>

This was referenced Feb 16, 2021

v15.9.0 proposal #37405

Closed

v15.9.0 proposal #37406

Merged

targos pushed a commit that referenced this pull request May 1, 2021

tools: add GitHub Action linter for pr-url

981659c

PR-URL: #37221 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Rich Trott <[email protected]>

danielleadams mentioned this pull request May 3, 2021

v14.17.0 proposal #38507

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: add GitHub Action linter for pr-url #37221

tools: add GitHub Action linter for pr-url #37221

aduh95 commented Feb 4, 2021 •

edited

Loading

richardlau left a comment

aduh95 commented Feb 4, 2021 •

edited

Loading

RaisinTen Feb 4, 2021

aduh95 Feb 4, 2021

RaisinTen left a comment

RaisinTen Feb 8, 2021

RaisinTen Feb 8, 2021

RaisinTen Feb 8, 2021

RaisinTen commented Feb 8, 2021

RaisinTen Feb 8, 2021

aduh95 Feb 8, 2021

aduh95 commented Feb 8, 2021 •

edited

Loading

RaisinTen commented Feb 8, 2021

aduh95 commented Feb 8, 2021

nodejs-github-bot commented Feb 8, 2021

nodejs-github-bot commented Feb 8, 2021

nodejs-github-bot commented Feb 9, 2021

aduh95 commented Feb 9, 2021

		import process from 'node:process';
		import readline from 'node:readline';

	import process from 'node:process';
	import readline from 'node:readline';
	import readline from 'readline';

	const validatePrUrl = (url) => url == null \|\| url === expectedPrUrl;
	const validatePrUrl = (url) => url === expectedPrUrl;

	} else if (!validatePrUrl(line.match(prUrlDefinition)?.[1])) {
	} else if (currentPrUrl = line.match(prUrlDefinition)?.[1] &&
	validatePrUrl(currentPrUrl)) {

	if: ${{ github.event.pull_request }}
	if: github.event.pull_request.draft == false

tools: add GitHub Action linter for pr-url #37221

tools: add GitHub Action linter for pr-url #37221

Conversation

aduh95 commented Feb 4, 2021 • edited Loading

richardlau left a comment

Choose a reason for hiding this comment

aduh95 commented Feb 4, 2021 • edited Loading

RaisinTen Feb 4, 2021

Choose a reason for hiding this comment

aduh95 Feb 4, 2021

Choose a reason for hiding this comment

RaisinTen left a comment

Choose a reason for hiding this comment

RaisinTen Feb 8, 2021

Choose a reason for hiding this comment

RaisinTen Feb 8, 2021

Choose a reason for hiding this comment

RaisinTen Feb 8, 2021

Choose a reason for hiding this comment

RaisinTen commented Feb 8, 2021

RaisinTen Feb 8, 2021

Choose a reason for hiding this comment

aduh95 Feb 8, 2021

Choose a reason for hiding this comment

aduh95 commented Feb 8, 2021 • edited Loading

RaisinTen commented Feb 8, 2021

aduh95 commented Feb 8, 2021

nodejs-github-bot commented Feb 8, 2021

nodejs-github-bot commented Feb 8, 2021

nodejs-github-bot commented Feb 9, 2021

aduh95 commented Feb 9, 2021

aduh95 commented Feb 4, 2021 •

edited

Loading

aduh95 commented Feb 4, 2021 •

edited

Loading

aduh95 commented Feb 8, 2021 •

edited

Loading