Proposal to update v7.x-staging #10989
Conversation
PR-URL: nodejs#10419 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Refs: nodejs#9901 (comment)
1. equal => strictEqual. 2. let => const for the variable that is not reassigned. 3. fix spaces. 4. stringify erroneous raw buffer outputs. 5. fix a typo. PR-URL: nodejs#10102 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Unlike all the other tls APIs, if any secure context configuration is required, the caller is responsible for creating the context. Corrects a doc regression introduced in caa7fa9. PR-URL: nodejs#10545 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Standardise on Refs: PR-URL: nodejs#10670 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yorkie Liu <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Sam Roberts <[email protected]>
Added a note about Visual Studio 2015 feature which should be installed for building Node.js source code. PR-URL: nodejs#10669 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: Myles Borins <[email protected]>
`killSignal` option accepts the signal name or signal number as well. PR-URL: nodejs#10424 Reviewed-By: Julian Duque <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sam Roberts <[email protected]>
Corrected parameter for running tests on Windows. Without the corrected parameters, Windows users encounter an error about failing to sign the build, "Failed to sign exe", which can be discouraging to new Windows community members. PR-URL: nodejs#10686 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Bartosz Sosnowski <[email protected]>
The COLLABORATOR_GUIDE was still listing v0.10 and v0.12 as LTS when they are EOL now. PR-URL: nodejs#10720 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: James M Snell <[email protected]>
git apply does not preserve the original commit message. These updated instructions offer a simpler flow for backporting. PR-URL: nodejs#10665 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michal Zasso <[email protected]>
Changing `..can result in expected` to `..can result in unexpected` Fixes: nodejs#10710 PR-URL: nodejs#10712 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
The guide for writing tests is missing information on how tests are named. This adds that information. There is also some copy-editing done on the first paragraph of the guide. PR-URL: nodejs#10584 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Italo A. Casas <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Sam Roberts <[email protected]>
The doc specified that writable.write() was advisory only. However, ignoring that value might lead to memory leaks. This PR specifies that behavior. Moreover, it adds an example on how to listen for the 'drain' event correctly. See: nodejs@f347dad PR-URL: nodejs#10631 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
PR-URL: nodejs#10616 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Prince John Wesley <[email protected]>
Add the nodejs/url github team to the table of people to /cc for reviews on the WHATWG URL code. PR-URL: nodejs#10652 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
The note following the http.Server example in the vm documentation contains misleading language. This commit removes the incorrect reference to threads. Fixes: nodejs#10697 PR-URL: nodejs#10708 Reviewed-By: Brian White <[email protected]> Reviewed-By: James M Snell <[email protected]>
* Remove passive voice * Remove unneeded modifiers * Minor comma change PR-URL: nodejs#10585 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Italo A. Casas <[email protected]>
Docs referred to an `issuer` property being optionally present, when it should have referred to the `issuerCertificate` property. PR-URL: nodejs#10389 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Fix the explanation which stated that write() would return false if highWaterMark is exceeded to correctly state that false is returned once highWaterMark is reached. See nodejs#9247. PR-URL: nodejs#10582 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
PR-URL: nodejs#10828 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: James M Snell <[email protected]>
* Clarify that memory is always shared and never copied. * Fix wording that sounded like ArrayBuffer has a buffer property. PR-URL: nodejs#10778 Ref: nodejs#10770 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#10826 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#10826 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
This contained a duplicate link to the PR for a notable change, presumably because that PR was composed of 2 separate commits. PR-URL: nodejs#10827 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#10827 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
Add links to the engine classes for the zlib single-call convenience methods. PR-URL: nodejs#10829 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
This commit clarifies variables in the Filesystem docs. Prior, the documentation for fs.write() had an ambiguous remark on the parameters of offset and length. Therefore, this commit makes explicit that the length parameter in fs.write() is used to denote the number of bytes, which is a clearer reference for its usage. PR-URL: nodejs#9792 Ref: nodejs#7868 Reviewed-By: Sam Roberts <[email protected]>
PR-URL: nodejs#10883 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Italo A. Casas <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
PR-URL: nodejs#10869 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Evan Lucas <[email protected]>
PR-URL: nodejs#10954 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Changed authors listing from `Noah Rose` to `Noah Rose Ledesma`. PR-URL: nodejs#10945 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
|
@italoacasas You are missing
|
|
@Fishrock123 @cjihrig @evanlucas can someone of you guys LGTM this PR ? |
|
Did you get all of the commits @sam-github referenced? Did you run the CI against this? |
Document this with the YAML meta-data. PR-URL: nodejs#10983 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
The Hmac binding layer is not documented as part of the API, and is not intended to be used, but it should be robust to misuse, and contains defensive checks for misuse. This test checks that updates without init throw (as opposed to abort or misbehave in some other way). PR-URL: nodejs#10923 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>
@cjihrig running CI now, we are going to have two linter issues in the CI, this backport #11009 should resolve both. |
OK, I don't think we should land anything on the staging branch until that is resolved, but it's still good to see if the tests pass. |
|
Maybe we can land the backport in my branch instead staging ? |
Formatting changes for upcoming linter update. PR-URL: nodejs#10561 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sam Roberts <[email protected]>
We have been stalled on ESLint 3.8.0 for some time. Current ESLint is 3.13.0. We have been unable to upgrade because of more aggressive reporting on some rules, including indentation. ESLint configuration options and bugfixes are now such that we can reasonably upgrade. PR-URL: nodejs#10561 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sam Roberts <[email protected]>
As per ESLint documentation, http://eslint.org/docs/user-guide/configuring#configuration-file-formats the file format .eslintrc is deprecated. This patch just renames the files to .yaml and the structure is already in yaml format. PR-URL: nodejs#7699 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Prince John Wesley <[email protected]> Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: Rich Trott <[email protected]>
* use common.mustCall() as appropriate * eliminate exit handler * var -> const/let * provide duration for setInterval() PR-URL: nodejs#10588 Reviewed-By: Italo A. Casas <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Use common.mustCall() where appropriate, var to const/let, assert.equal() -> assert.strictEqual(), explicit time provided to setTimeout() PR-URL: nodejs#10551 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
italoacasas
force-pushed
the
v7.x-staging-1
branch
from
791c262
to
15a2e12
Compare
|
I'd like to backport the Fixes:
|
|
So I'd like to raise that this being the first time we have relied on a PR for updating the staging is proving to be a bit of a blocker to actually getting things in. How do people feel about how we've been doing it in the past. More liberal landing policy. The branch is not a "source of truth" and as such can be force pushed to fix problems. |
|
I'd always understood that I don't really see the need to have two different PRs, one to merge into IMHO the person who is doing the release (be it @italoacasas or someone else) should work on As I understand it the actual merging into |
|
Yea, I prefer the way we have done it in the past where we cherry pick what we can (that is 100% working and CI passes) and then use backport PRs for stuff that needs fixing up. |
|
I want to add something. I made the PR as a recommendation since this is my first time doing this I had more room to make mistakes and get feedback in the way I was doing the cherry-picks, etc... |
|
so I think it is worth mentioning that this has been a good exercise for learning how to use the tools and land the right stuff. @ItaloCasas I would like to encourage you to go ahead and simply land the changes onto v7.x-staging if you feel comfortable. a further audit can be done during release |
|
jinx 😜 tbqh I was thinking this could be a good process to follow for the sake of auditing backport. I'm still thinking that we may want to consider backport prs, but not require sign off |
|
I have a theory in progress where this PR can be made weekly by a bot that creates the PR and a list of commits that needs backport... |
|
@italoacasas let me know when |
|
Let's wait for @cjihrig green light too since he was mentoring me on this. |
|
@italoacasas go for it! |
|
@gibfahn ready. Closing this. |
|
@italoacasas I know this is closed, but I want to add wrt.
This is because you are skipping, for reasons I don't know, many commits. There were 3 commits to that file before my PR that are present on master and not present on your branch. You need to pull every single non-semver-major commit from master, in order. Personally, I find these lists of the commits included to not be helpful, what I want to know is what commits are not being pulled into Current from master. As I understand, every commit, if it isn't major, should make it down, or have a pretty clear justification as to why not. |
|
@sam-github, I include the list as a request from other member. This is the command that I run to get commits from master. branch-diff v7.x-staging master --exclude-label=semver-major,dont-land-on-v7.x --filter-release --reverse --format=sha | xargs git cherry-pickIf for some reason I cannot use the command because breaking test or something else, I do the fallowing. 1- Make a list of all the commits branch-diff v7.x-staging master --exclude-label=semver-major,dont-land-on-v7.x --filter-release > temp.md2- On your specific example, one problem is a |
|
right, those auto-added |
This is an(other) intent to update v7.x-staging... This time I cherry pick manually to detects the commits breaking the tests and the linter... I made lists by topics...
Note: This branch has two linter errors but I don't find the commit that introduces them, I was planning to deal with that later on before landing this on v7.x-staging