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

Confusion around semver-minor commits #820

Closed
anonrig opened this issue Feb 3, 2023 · 11 comments
Closed

Confusion around semver-minor commits #820

anonrig opened this issue Feb 3, 2023 · 11 comments

Comments

@anonrig
Copy link
Member

anonrig commented Feb 3, 2023

In the latest v19, there was a discussion around whether all semver minors are considered as a notable change, and later a semver minor change was added as a notable change(example: adding buffer.isAscii)

Today, I realized that the latest v18 release did not include semver minor changes in the notable changes section. (https://github.com/nodejs/node/releases/tag/v18.14.0)

What is the procedure about this? Collaborators have a conflict about this issue, and it would help a lot if the releasers solve this conflict.

Thank you!

@anonrig
Copy link
Member Author

anonrig commented Feb 3, 2023

I can't add the agenda label, but I appreciate if someone can add it.

@RafaelGSS
Copy link
Member

That's usually up to the releaser to decide. If there's no notable-change label a semver-minor might or might not be added as a notable change, except in the case of major releases https://github.com/nodejs/node/blob/main/doc/contributing/releases.md#generate-the-notable-changes.

Sometimes we forget and sometimes is just hard to define if a commit is notable or not without having the label explicitly in the PR. Therefore, I suggest if you feel your PR is notable, add the label.

@ljharb
Copy link
Member

ljharb commented Feb 3, 2023

Ideally all semver-minors would be considered notable by default, unless it was a special case that wasn't really notable.

Could the label instead be "this minor change is NOT notable", so that minor changes are included by default?

@targos
Copy link
Member

targos commented Feb 3, 2023

Ideally all semver-minors would be considered notable by default, unless it was a special case that wasn't really notable.

I agree with that, and git node release --prepare (which I always use) generates a changelog based on this assumption.

@BethGriggs
Copy link
Member

This was discussed in the v18.14.0 proposal yesterday (nodejs/node#46396 (comment))

@tniessen, generally we've defaulted to elevating all semver minors to notable, but only demote them in the cases we're certain they really are not useful to highlight to end-users.

This was the way I (and I suspect others) were advised to craft notable changes during the release onboarding process. It's alluded to in releases.md - but our release automation also currently makes the same assumption when generating the notable changes (nodejs/node-core-utils/blob/main/lib/prepare_release.js).

I think it is a fair order of operations to consider non-notable semver minors the exception - especially considering releasers do not always have the full context of the commits.

@BethGriggs
Copy link
Member

Today, I realized that the latest v18 release did not include semver minor changes in the notable changes section. (https://github.com/nodejs/node/releases/tag/v18.14.0)

For v18.14.0 @juanarbol was asked to mark some minors as not notable in both nodejs/node#46396 (comment), nodejs/node#46396 (review).

It seems nodejs/node#45947 is the only minor that wasn't explicitly asked to be demoted. It seems to have got lost somehow - it wasn't mentioned this list (nodejs/node#46396 (review)). If you prefer nodejs/node#45947 to be marked as notable we can retroactively update the changelog and website blogpost (we have done so in the past).

@richardlau
Copy link
Member

Could the label instead be "this minor change is NOT notable", so that minor changes are included by default?

The label is not specific to semver minors... it can also be applied to semver patch PRs if the bug being fixed is significant.

@anonrig
Copy link
Member Author

anonrig commented Feb 3, 2023

That's usually up to the releaser to decide.

@RafaelGSS I understand that it's up to the releaser, but I think releasers should not make conflicting decisions about the same pull request on different releases. The sentence in Use your judgment there. I strongly believe that we should remove and/or change this sentence from the release contributing document: Some SEMVER-MINOR commits may be listed as notable changes on a case-by-case basis. Use your judgment there.

Ideally all semver-minors would be considered notable by default, unless it was a special case that wasn't really notable.

@ljharb This is not mentioned in the release document. The document states that Some SEMVER-MINOR commits may be listed as notable changes on a case-by-case basis. Use your judgment there., and this is why it is causing conflicts.

If you prefer nodejs/node#45947 to be marked as notable, we can retroactively update the changelog and website blogpost (we have done so in the past).

@BethGriggs Thank you for the offer. It would be unfair to other semver-minor commits, which should be treated as same as the referring commit. So, I don't think we should do it.


I think it would be beneficial to have a written decision/rule about this so that there won't be any confusion. Due to the nature of the label notable, it is and will create emotional conflicts for collaborators. On top of that, as a non-native speaker, the word notable plays an important part in these kinds of conflicts. I strongly believe that it sends a wrong message (and demotivates) when a label is marked notable-change and later removed by a TSC member because the requirements for a notable-change is not clearly defined in the release guidelines.

PS: I don't have a strong opinion towards the decision, but it does not feel right to me to see the same commit marked as a notable change in v19 and not in v18.

@ljharb
Copy link
Member

ljharb commented Feb 3, 2023

@anonrig that makes sense - i'm not familiar with the release document. my comment was aspirational tho, so i'd be happy to see the document updated :-) in particular, it's nice to not have to look through the commit/PR list in order to figure out what's added.

@BethGriggs
Copy link
Member

Releaser's approach (and as what is implemented by our tooling today) was clarified in nodejs/node#46592:

By default, the ### Notable changes section should be populated with the
commits in the release that have either the notable-change or semver-minor
label. Some semver-minor features may be determined by the releaser, or
indicated by another contributor, to not be appropriate to be listed as a
notable. The ultimate decision rests with the releaser.

@anonrig
Copy link
Member Author

anonrig commented Mar 10, 2023

Thanks for the update @BethGriggs. I'm closing this issue for now.

@anonrig anonrig closed this as completed Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants