Role of CTC in semver-major changes needs clarification

[Trott]

In light of #7846:

The onboarding doc indicates that all semver major changes must be reviewed by CTC "in some form". I think this could use some review and clarification:

• It's not clear what "in some form" entails.
• As far as I know, the onboarding doc is the only place this requirement appears. If it's not misinformation, then it should be documented elsewhere. (Maybe it is and I just haven't noticed it?)
• We don't appear to be sticking to this, except in the loosest conceivable interpretation of "in some form". See fs: remove maybeCallback function #7168.

I think the CTC should decide:

• Does the CTC need to review all semver-major changes?
• If so, then can we get some kind of specifics about what that means?
• If not, then we should remove that bit of information from the onboarding doc.

It's not clear to me that more CTC oversight would have prevented the issue that has come up with that change. I'm not advocating for anything here, other than clarifying the process. I'm fine with more formal CTC oversight on semver-major issues and I'm fine with eliminating the requirement. I just want clarity and consistent application of whatever rules are decided upon.

@nodejs/ctc @thealphanerd

[indutny]

In my opinion, CTC should formally review breaking changes, unless that is a V8 update.

[jasnell]

Yes, CTC members should review and sign off on all semver-major changes.
There should be at least one lgtm from a CTC member.

On Friday, July 22, 2016, Rich Trott notifications@github.com wrote:

In light of #7846 #7846:

The onboarding doc indicates that all semver major changes must be
reviewed by CTC "in some fashion"
https://github.com/nodejs/node/blob/b3127df59ab23baa68e915d62ea1997adb5669e0/doc/onboarding.md#process-for-getting-code-in.
I think this could use some review and clarification:

• It's not clear what "in some form" entails.
• As far as I know, the onboarding doc is the only place this
  requirement appears. If it's not misinformation, then it should be
  documented elsewhere. (Maybe it is and I just haven't noticed it?)
• We don't appear to be sticking to this, except in the loosest
  conceivable interpretation of "in some form". See fs: remove maybeCallback function #7168
  fs: remove maybeCallback function #7168.

I think the CTC should decide:

• Does the CTC need to review all semver-major changes?
• If so, then can we get some kind of specifics about what that means?
• If not, then we should remove that bit of information from the
  onboarding doc.

It's not clear to me that more CTC oversight would have prevented the
issue that has come up with that change. I'm not advocating for anything
here, other than clarifying the process. I'm fine with more formal CTC
oversight on semver-major issues and I'm fine with eliminating the
requirement. I just want clarity and consistent application of whatever
rules are decided upon.

@nodejs/ctc https://github.com/orgs/nodejs/teams/ctc @thealphanerd
https://github.com/TheAlphaNerd

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#7848, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAa2edeEEDDKx7Pbn2hAN8xAVDgubl7yks5qYYvygaJpZM4JTRcK
.

[bnoordhuis]

There should be at least one lgtm from a CTC member.

That happened, didn't it? Trevor signed off on both #2498 and #7168.

[Fishrock123]

There should be at least one lgtm from a CTC member.

An ok baseline but really we want the CTC as a group to have their eyes on these not just individuals. I don't see a huge problem with the current wording in that document.

---

The actual problem here is our dev policy docs never actually got merged into this repo during the io.js merge, so the official policy is under here:

Pull Requests that require an increase in the Major version must be elevated for review by the TSC. This does not necessarily mean that the PR must be put onto the TSC meeting agenda. If enough TSC members sign-off on the PR and there is clear consensus among TSC members for the change, the Pull Request can be landed. Where there is disagreement among TSC members, semver-major Pull Requests should be put on the TSC meeting agenda.

Note: this language is outdated and was written before the TSC/CTC split, but you get the idea.

[misterdjules]

Was the problem in #7846 a code-review problem, or a testing problem? It seems that for both #2498 and #7168, citgm was run only after these changes landed. EDIT: As @thealphanerd pointed out, citgm wasn't run only for #2498.

If I understand correctly, citgm caught at least some of these issues when it ran.

While I'm in favor of clarification of the code review process for semver-major changes, if the problem we're trying to solve here is to avoid the regressions described in #7846, I think we want to look at when we use citgm too.

Code reviews without running citgm (and possibly other tests) are not enough to get a good idea of the impact of a code change on the ecosystem.

It could be that the process would mention something along the lines of: "a member of the CTC must run the citgm tests suite, and check that all tests pass, before landing any semver-major change".

[MylesBorins]

@misterdjules citgm was run on #7168, but after it was run more changes were made. It likely should have been run again before it landed.

In general I think passing citgm should be a minimum bar for any semver major change

[misterdjules]

@misterdjules citgm was run on #7168, but after it was run more changes were made. It likely should have been run again before it landed.

@thealphanerd My apologies for the confusion, my artisanal search method failed to catch that. Thank you for pointing this out!

[Trott]

I think incorporating a modified version of the text @Fishrock123 quotes would be a good move. How's this?:

Pull Requests that require an increase in the Major version must be elevated for review by the CTC. This does not necessarily mean that the PR must be put onto the CTC meeting agenda. If multiple CTC members approve (LGTM) the PR and no Collaborators oppose the PR, it can be landed. Where there is disagreement among CTC members or objections from one or more Collaborators, semver-major Pull Requests should be put on the CTC meeting agenda.

@jasnell suggested that at least one CTC member should LGTM the PR. This wording requires at least two CTC members. Otherwise, I think this is consistent with all the other comments.

Also: Yeah, I'm not necessarily saying anything went wrong with the way that PR landed. I'm saying that things are currently vague. I'm usually a fan of not getting too specific if it's not necessary, but in this case, I think it is warranted.

[jasnell]

I'm good with this, albeit for some of the more specialized parts of core, two lgtms may be more difficult to come by.

[Trott]

I've opened #7955 to document this change. Hopefully we can get enough CTC members to LGTM that to land it. In the meantime, I'll remove the ctc-agenda label from this issue and take it off the meeting agenda as discussion can happen in that PR. We can always put it back on the agenda for next week if it turns out there's no consensus.