Skip to content

Conversation

@codebytere
Copy link
Member

@codebyterecodebytere commented May 20, 2020

Adds an additional step in git node land to automatically update any DEP0XXX, DEP0XX1 etc tags in the codebase to incremented new numbers. The release preparation script will also check for unmarked numbers and optionally update them as well.

Tested locally but please double check for any potential edge cases i might have missed!

cc @targos

@codebyterecodebytere requested a review from targosMay 20, 2020 06:53
@codecov
Copy link

codecovbot commented May 20, 2020

Codecov Report

Merging #420 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@ Coverage Diff @@## master #420 +/- ## ======================================= Coverage 77.13% 77.13% ======================================= Files 21 21 Lines 1496 1496 ======================================= Hits 1154 1154 Misses 342 342 

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7dc544d...a3c6eb7. Read the comment docs.

@codebyterecodebytereforce-pushed the auto-update-deprecations branch from 2ce77f4 to 3a8e38cCompareMay 20, 2020 06:56
@richardlau
Copy link
Member

It would be better if this was part of the git node land <pr> workflow to stop DEP0XXX landing in the first place. Typically we'd fix those up in master before backporting (because I think we want the numbers to be consistent across master/release lines). I guess we could fix these at release time as long as the change is merged upstream afterwards (hopefully without conflicts).

@codebytere
Copy link
MemberAuthor

codebytere commented May 20, 2020

@richardlau good point - i added this since it came up yesterday during the v14 release with this PR, but a second pass of the releases guide shows:

This assignment should occur when the PR is landed, but a check will be made when the release build is run.

I'll migrate this logic into the land logic!

@richardlau
Copy link
Member

@richardlau good point - i added this since it came up yesterday during the v14 release with this PR, but a second pass of the releases guide shows:

This assignment should occur when the PR is landed, but a check will be made when the release build is run.

I'll migrate this logic into the land logic!

If you want to also codify the check during release (in addition to modifying the land logic) I'd have no objection 😁.

@codebyterecodebytereforce-pushed the auto-update-deprecations branch 2 times, most recently from e40a212 to 2f0b952CompareMay 20, 2020 18:39
@codebyterecodebytere changed the title feat: update DEP0XXX tags during release prepfeat: handle unmarked DEP0XXX tags during landing and releaseMay 20, 2020
@codebyterecodebytereforce-pushed the auto-update-deprecations branch from 2f0b952 to f177b58CompareMay 26, 2020 16:48
@codebyterecodebytereforce-pushed the auto-update-deprecations branch from f177b58 to 53a4c31CompareMay 30, 2020 16:39
@codebyterecodebytere requested a review from BridgeARJune 1, 2020 16:57
@codebytere
Copy link
MemberAuthor

@targos i'd love to be able to get this in before next week's release if you have some time to peek at it 🙇‍♀️

@targos
Copy link
Member

I'm afraid this cannot be easily automated, because there may be multiple DEP0XXX placeholders for different deprecations introduced in different PRs. nodejs/node#33294 was special because it introduced more than one deprecation in a single PR.
I prefer an approach where we prevent placeholders to land at all (something nodejs/node#33433 is attempting to do).

@codebytere
Copy link
MemberAuthor

codebytere commented Jun 3, 2020

@targos in that case - shall i just adapt this PR to check for unmarked DEP0XXX tags and not try to fix them?

@targos
Copy link
Member

What about a compromise : fix it if there is only one placeholder in deprecations.md, otherwise pause and ask to fix manually?

@codebytere
Copy link
MemberAuthor

codebytere commented Jun 3, 2020

@targos i've thought about this a bit more - i think it would still make sense to automate multiple in the landing stage? I can't think of a case where there will be two conflicting deprecations introduced in the same PR 🤔

In the release stage it makes sense only to automate if there's one though, i agree! I bumped some related changes and if there's an edge case i'm missing i'll remove the multiple functionality from landing!

@codebyterecodebytere requested a review from targosJune 4, 2020 16:15
Copy link
Member

@targostargos left a comment

Choose a reason for hiding this comment

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

@codebyterecodebytere merged commit 08bc3fc into nodejs:masterJun 7, 2020
@codebyterecodebytere deleted the auto-update-deprecations branch June 7, 2020 17:50
codebytere added a commit to codebytere/node-core-utils that referenced this pull request Jun 8, 2020
…#420) Adds an additional step in git node land to automatically update any DEP0XXX, DEP0XX1 etc tags in the codebase to incremented new numbers. The release preparation script will also check for unmarked numbers and optionally update them as well.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@codebytere@richardlau@targos