-
Notifications
You must be signed in to change notification settings - Fork 578
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
README redo #121
README redo #121
Conversation
<td>N/A</td> | ||
<td>v5</td> | ||
<th>v5</th> | ||
<td>Current (non-LTS)</td> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think this should be "Maintenance" (non-LTS)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK with this but I was being vague about it because so far we've resorted to leaving a label off v5 after we got v6. Having it explicitly called "Maintenance" would probably be a step up from that and clarify how it works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it's no longer Current
so maybe literally call it
¯\_(ツ)_/¯
I think we should define ABI. |
It links to the Wikipedia article. Are you thinking of something more specific? |
|
||
Release lines in "Current" _may_ receive updates to their V8 dependency. This will occur ***only if*** it can be done in a way that is not semver-major and does not break the stable ABI of the current major version. This may be achieved by either receiving an ABI-stable update from upstream or by applying floating patches to the dependency to ensure ABI-stability. Implementation details are left up to the Node.js V8 team. | ||
|
||
Upgrades to V8 in a "Current" release line should not occur within two months of that release line moving in to "Active LTS". i.e. upgrades to V8 should not occur from August through to the transition of "Current" into LTS in October. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is somewhat ambiguous because August through to October would be three months.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ofrobots actually I was thinking of expanding on the ambiguity and leaving it up to both the LTS WG and the nodejs/v8 team to negotiate on exact timing rather than being strict about it in this document. Since we may not get an LTS till late October then perhaps having a late August V8 upgrade isn't a big deal. It also comes down to the nature of the V8 upgrade at that time, perhaps it's very minor or perhaps it does something big like replace the GC or something that changes the performance profile in a major way or introduces new risks that we're concerned about.
Would you be OK with introducing explicit flexibility in here and make it guidance rather than a rule?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or just change the wording to "For example". I think just saying should not within 2 months is reasonable as the intention and then as everything there can be exceptions. Then change the last sentence to "For example, assuming LTS ships at the end of October upgrades should not occur between then end of August and that time."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to state it as an intent but allow for some flexibility as deemed appropriate by the LTS and V8 teams. I didn't parse the first sentence as offering much flexibility, and the second sentence ambiguously expands it to three months.
I think I agree with @Fishrock123 on defining ABI, or at least expanding it. It's a foreign concept for most Node users and I find myself always explaining it (or being corrected by people: "API you mean?"). I'll see if I can put a sentence or two along with the link for further info. |
I'd like to see some definition for the scope (or criteria) of backporting fixes. So far, the text tends to reference the downstreaming controlled by the classification of semver.[major | minor] only, and in practice by the use of the 'lts-watch' tags, for example. Another aspect to potentially consider is also how we are ensuring the quality of stability of the older releases. There is discussion of the structure of the LTS branches (staging and release). However, as far as I can determine, any changes to be delivered into any release are always received direct from the (unstable) Master stream? Meaning, before cutting a new Release (LTS or Maintenance) we are currently receiving fixes/changes that, subject to code review and blessing of an LTS workgroup member, may be only days (or hours) old that haven't had any timescale in which they have demonstrated stability (even in Master!)? Should there be a process defined where a change/fix being backported is allowed to 'settle' in a 'Current' release streams delaying the application to the 'mostly stable' LTS, and 'most-stable' Maintenance releases? Is this effectively what is happening under the current LTS regime somehow? Or by only considering Critical fixes (cf Security), that effectively no other fixes are/will ever result in a changed Maintenance release. |
My understanding is that we do have a "settling" time in the "Current" release before PR's are backported to LTS. I think adding some text to describe that would make sense. |
* Receive semver-patch commits, i.e. those not labelled either semver-major or semver-minor as appropriate. | ||
Any semver-patch commits proposed for inclusion in an LTS release line will require: | ||
- sign-off from at least one active member of the LTS Working Group; | ||
- no objection from any active member of the LTS Working Group |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we should include an explicit mention of CITGM here? No commit should land in an LTS that breaks anything in a CITGM run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My hesitance here is that citgm results are still flaky in a similar sense that we use that term to talk about the other core tests. See https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/ for example. Being strict about getting a citgm pass might mean it's nearly impossible to get things in, plus the burden of figuring out the nature of citgm failures is a non-trivial affair and I wouldn't want to make @thealphanerd a necessary part of the process simply because he's the most qualified to look at citgm results and give a 👍 or 👎.
Perhaps this is something to revisit later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point I'm ok with landing as is and considering adding a CITGM reference later on.
Yes, I think it's worthwhile mentioning that any non-trivial, non-doc and non-test related commits landing in master should sit through at least one current release cycle before landing in an LTS staging branch. |
Few comments, but overall LGTM |
Closing this due to lack of forward progress on it. Can reopen if necessary |
While I've taken many elements from the current README, this is basically a rewrite.
The main things I'm proposing here are:
I'd like to get some discussion amongst @nodejs/lts first before taking this to Collaborators and then CTC for sign-off.
FYI I did sentence-linebreaking in this doc as a compromise between char limit line breaks (which I hate) and full paragraph linebreaks which I prefer but are a bit more difficult to review in PR form. Feedback on that is welcome too of course!