Check DEP00XX and REPLACEME tags for PRs

[gibfahn]

See e.g. nodejs/node#18990 (comment) , I'm not sure where this is documented, although there is something here:

https://github.com/nodejs/node/blob/61e3e6d56feefcd88f2baeb59c31c327835a8d90/doc/releases.md#step-3-update-any-replaceme-and-dep00xx-tags-in-the-docs

PRs that add new deprecations should have DEP00XX, which should be replaced with an actual number (e.g. DEP0100 when it lands).

PRs that add new APIs should have REPLACEME tags, that are updated with the correct version when they land in a Current release (which is then backported to LTS versions).

Things we could do:

Basic:

• Warn if there is a DEP00XX tag.
• Autoreplace DEP00XX with the next deprecation number.

Complicated:

• Check for newly added DEP???? tags (where the PR erroneously contains an already-assigned deprecation number) and warn.
  • Also autoreplace with the correct number
• Check for added: v?.?.? tags in the diff and warn.
• Check for new APIs that don't have the added: option (should maybe be a node lint instead).

cc/ @nodejs/release @nodejs/lts as the groups perhaps most likely to know about these things.

[priyank-p]

For, checking if a DEP00XX is included in PR we need to make a call to github v3 api. I'd avoid making useless calls so how about we check if deprecation tag is checked if commit msg includes deprecate or deprecation?

(This is temporary until near future, since github api v4 team is already planning to implement this)

[gibfahn]

For, checking if a DEP00XX is included in PR we need to make a call to github v3 api. I'd avoid making useless calls so how about we check if deprecation tag is checked if commit msg includes deprecate or deprecation?

If this is done as part of git-node-land you should already have the commits on disk right?

[joyeecheung]

I feel like this should be done in the linter somehow? Or a pre-commit git hook. It feels a bit too late to warn about that during landing

[priyank-p]

I was thinking this to be done before landing the PR and let the reviewer know that this PR has a dep tag (correct/incorrect update), But if this was to be done locally or pre commit-hook how do we validate the DEP Tag AFAICT it should be only be DEP00XX regardless of the number of DEPS right?

[joyeecheung]

I guess we can grab the diff of a commit with message containing "deprecate"/"deprecation", and look for lines that add DEP\w+ - if there are no equivalent lines that remove the same code, that means this is a new deprecation, then we can go check if it's DEP00XX. Although there are still other things that we need to take care of...like for example we only need to check commits that land on the master branch.

OK so now this sounds more like the job of core-validate-commit? Or we can add another tool that does this during git-node-land --final

[gibfahn]

I feel like this should be done in the linter somehow? Or a pre-commit git hook. It feels a bit too late to warn about that during landing

Which thing specifically? The "replace DEP00XX tags" needs to be done as part of the landing process (at the same time as you modify the commit message to add the metadata), so it can't be done as a lint.

I think it probably makes sense as part of git node land --amend, but I haven't used git node land, so I could be wrong.

[joyeecheung]

@gibfahn I see, I thought you were talking about something like "warning against DEP100 code assigned on master branch because it should not be done there", but IIUC you were actually talking about landing commits on release branches. I have not used git node land on a release branch before but I think it is possible to run some extra stuff there (just need to ncu-config set branch v8.x-staging and it'll know it's landing commits on a release branch).

[gibfahn]

I thought you were talking about something like "warning against DEP100 code assigned on master branch because it should not be done there",

That is what I called the Complicated part. Both the Complicated and the Basic checks need to be done on landing, because the linter can't tell whether it's being run on a commit that has landed on master. It should be easy enough to run any checks as part of the git node land --amend.

but IIUC you were actually talking about landing commits on release branches.

The REPLACEME tags need to be done when landing commits on release branches, the DEP00XX tags need to be done when landing commits on master. We should probably deal with them separately.

(just need to ncu-config set branch v8.x-staging and it'll know it's landing commits on a release branch).

Couldn't we figure that out from the PR base branch?

OK so now this sounds more like the job of core-validate-commit? Or we can add another tool that does this during git-node-land --final

Could be done in either I guess. Is there a plan to pull core-validate-commit into this repo (with git subtree or something)?

[gibfahn]

See nodejs/node#18990 for a good example of something with both.

The DEP00XX will be replaced on landing by whoever lands the PR on master. The REPLACEME will be replaced on backporting by whoever backports the PR to v9.x.

[mmarchini]

#420 implemented this for DEP00XX, we just need to do the same for REPLACEME now.

[richardlau]

REPLACEME tags should only be set in the release commits. This is already done in

node-core-utils/lib/prepare_release.js

Lines 115 to 118 in 498d600

	// Update any REPLACEME tags in the docs.
	cli.startSpinner('Updating REPLACEME items in docs');
	await this.updateREPLACEMEs();
	cli.stopSpinner('Updated REPLACEME items in docs');

.

[mmarchini]

Oh cool, we can close then