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

Clarifying breaking changes landing in Current or LTS #660

Closed
Trott opened this issue Jan 29, 2019 · 7 comments
Closed

Clarifying breaking changes landing in Current or LTS #660

Trott opened this issue Jan 29, 2019 · 7 comments

Comments

@Trott
Copy link
Member

Trott commented Jan 29, 2019

Our current documentation in the Collaborator Guide says:

* Breaking changes should *never* land in Current or LTS except when:
  * Resolving critical security issues.
  * Fixing a critical bug (e.g. fixing a memory leak) requires a breaking
    change.
  * There is TSC consensus that the change is required.

The question is:

  • Is the last bullet point correct? Is it @nodejs/tsc that gets to decide this or has this been delegated to @nodejs/release via their charter? I'd argue that it's been delegated based on the bullet points at https://github.com/nodejs/TSC/blob/master/WORKING_GROUPS.md#release and the mandate in https://github.com/nodejs/release. As such, TSC cannot take back that authority without de-chartering the Release WG. Ergo, TSC consensus is not what allows a breaking change to land on an LTS or Current branch. Rather, it is Release WG consensus that permits that. Agreed?
@Trott
Copy link
Member Author

Trott commented Jan 29, 2019

By the way, I think the last bullet point should just be removed. The first two bullet points are consistent with the contents of the https://github.com/nodejs/release README file, so they can stay.

Trott added a commit to Trott/io.js that referenced this issue Jan 29, 2019
The TSC has delegated authority over LTS and Current branches to the
Release WG. Remove the bullet point about TSC having authority to
determine that a breaking change is necessary on LTS and Current release
branches. Retaining that authority would require de-chartering the
Release WG.

Fixes: nodejs/TSC#660
@Trott
Copy link
Member Author

Trott commented Jan 29, 2019

Proposed fix: nodejs/node#25780

There's likely room to add in mention that the Release WG ultimately manages the contents of the LTS and Current branches, but probably not in a bullet point here? Anyway, that can be a different commit, perhaps added by a Release WG member to make sure it is worded exactly the way they want?

@mhdawson
Copy link
Member

I'm not sure that it's a black or white situation where once delegated an area has 100% control so I think if the right answer is that the TSC needs to approve then that should be ok. ie. Release team has full autonomy except that the TSC should be consulted for breaking changes in LTS releases (or some other variant for example at least an FYI).

So forgetting technicalities etc. is the right answer that the TSC does not need to be consulted or get a heads up when a breaking change goes into an LTS release?

@Trott
Copy link
Member Author

Trott commented Jan 31, 2019

So forgetting technicalities etc. is the right answer that the TSC does not need to be consulted or get a heads up when a breaking change goes into an LTS release?

Forgetting technicalities: IMO the Release team does not need to do any of that. I mean, look at issues like nodejs/node#23974 listing every change, every commit, clearly labeling semver-minor and semver-major, etc. That's more than enough "heads up" and consultation. And the Release team and TSC teams overlap. Semver majors aren't going to slip in unnoticed by the TSC. Adding this requirement will just be one more checkbox (and a low-to-zero-value one at that) for the already overly burdensome and manual Release process.

@Trott
Copy link
Member Author

Trott commented Jan 31, 2019

I guess it's also worth pointing out that:

  • A semver-major won't land in LTS or Current without a ton of discussion in places that TSC is going to notice, like the security repo or somewhere like that. And it is all but certain that Release will ping TSC anyway to let them know.

  • Given the above bullet point, I guess you could also make the argument that codifying TSC-notification and/or TSC-approval doesn't change much so it's worth doing. I personally don't find that argument compelling because, as the discussions in doc: remove outdated COLLABORATOR_GUIDE sentence about breaking changes node#25780 and here demonstrate, we already have too many rules that we have confusion about and perhaps even a general lack of agreement on. More rules exacerbates the need for conversations like this one. We need fewer, simpler, and more consistent rules.

  • Given that bullet point, you could argue for asking Release WG to either accept a charter/mandate change or to simply impose a rule themselves (which they may already have?) saying that semver majors don't land in LTS or Current without TSC approval, since that arguably simplifies things to "TSC approval needed for unusual stuff". I'd rather we streamline our collaborator docs and everyone agree that Release WG determines what goes into releases, since that's both simpler and consistent with the current status of things. But, as evidenced by this comment, reasonable people can certainly make arguments otherwise.

@mhdawson
Copy link
Member

I don't have a strong opinion. "It doesn't change much" and "a heads up does not hurt" resonates with me but I think we should go with what the Release team and TSC as a group thinks is right.

addaleax pushed a commit to nodejs/node that referenced this issue Feb 1, 2019
The TSC has delegated authority over LTS and Current branches to the
Release WG. Remove the bullet point about TSC having authority to
determine that a breaking change is necessary on LTS and Current release
branches. Retaining that authority would require de-chartering the
Release WG.

Fixes: nodejs/TSC#660

PR-URL: #25780
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@ChALkeR
Copy link
Member

ChALkeR commented Feb 5, 2019

@Trott is there still anything left to do here?

@Trott Trott closed this as completed Feb 5, 2019
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

Successfully merging a pull request may close this issue.

4 participants