search

nodejs / node-core-utils

CLI tools for Node.js Core collaborators
https://nodejs.github.io/node-core-utils/
MIT License
234 stars 106 forks source link

Check DEP00XX and REPLACEME tags for PRs #193

Closed gibfahn closed 3 years ago

gibfahn commented 6 years ago

See e.g. https://github.com/nodejs/node/pull/18990#discussion_r170592279 , 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:

Complicated:

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

priyank-p commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

See https://github.com/nodejs/node/pull/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 commented 3 years ago

https://github.com/nodejs/node-core-utils/pull/420 implemented this for DEP00XX, we just need to do the same for REPLACEME now.

richardlau commented 3 years ago

REPLACEME tags should only be set in the release commits. This is already done in https://github.com/nodejs/node-core-utils/blob/498d600a4382d731f40327a5ce0bd4738961926b/lib/prepare_release.js#L115-L118.

mmarchini commented 3 years ago

Oh cool, we can close then