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

Add Travis CI and AppVeyor coverage for Node 5.11 and Node 6.1 #2996

Closed
wants to merge 2 commits into from
Closed

Add Travis CI and AppVeyor coverage for Node 5.11 and Node 6.1 #2996

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented May 13, 2016

Let me know if there are any issues.

Add Travis CI coverage for Node 6.1
@LinusU
Copy link
Member

LinusU commented May 13, 2016

LGTM

@LinusU
Copy link
Member

LinusU commented May 13, 2016

@dougwilson Any reason why we aren't specifying just major?

@dougwilson dougwilson added the pr label May 13, 2016
@dougwilson
Copy link
Contributor

dougwilson commented May 13, 2016

Hi @calvinmikael, just as a first though, you are missing the corresponding additions to the AppVeyor configuration as well.

The addition of the 6.x line takes extra consideration, and I'm going to pull from #2757, #2751 and all kinds of other various previous PRs for the following:

  1. Before we can add any new versions to our Travis CI, we need to ensure all of our sub dependencies are fully functional on those versions first. There are too many sub dependencies to list here, but you can find a list in the packagee.json file (https://github.com/strongloop/express/blob/master/package.json). Once every one is tested and updated to officially support Node.js 6.x, we can proceed.

  2. We also need to examine the changelog of Node.js/io.js on majors to determine what is different, what the breaking changes are, and how do they relate to what this module does (unified changelog at https://github.com/nodejs/node/blob/master/CHANGELOG.md). This module deals with details of HTTP. Please verify this and let me know.

Let me know as your progress through this and if you get stuck or anything!

As for @LinusU, the reason they are pinned to the minor is to prioritize reproducible builds, at the expense of a very simple chore. There was a flurry of discussion about this prior to the TC and this pattern was the result. I think if we desire further discussion or re-evaluation, we just make a issue in the discussions repo or add it to TC agenda :)

Add AppVeyor Coverage for Node 6.1
@ghost ghost changed the title Add Travis CI coverage for Node 5.11 and Node 6.1 Add Travis CI and AppVeyor coverage for Node 5.11 and Node 6.1 May 13, 2016
@ghost
Copy link
Author

ghost commented May 13, 2016

@dougwilson
I made a commit that adds Node 5.11 and Node 6.1 to AppVeyor.

I'd like to clarify if the sub dependency verification also includes dev dependencies.
I'd also like to clarify whether your meaning of update includes a new published npm version of all dependencies or only merged commits to their git master repositories.
I'd appreciate another point of clarification for whether dependency tests need to be pinned to the minor version or if only the major is required.

accepts
PR submitted
https://github.com/jshttp/accepts/blob/master/.travis.yml
array-flatten
PR submitted
https://github.com/blakeembrey/array-flatten/blob/master/.travis.yml
content-disposition
PR submitted
https://github.com/jshttp/content-disposition/blob/master/.travis.yml
content-type
cookie
cookie-signature
debug
depd
escape-html
etag
finalhandler
fresh
merge-descriptors
methods
on-finished
parser
path-to-regexp
proxy-addr
qs
range-parser
send
server-static
type-is
utils-merge
vary

I'll list below all changes to http/https in Node 5.0-5.11 and Node 6.0-6.1 however while I don't see any breaking changes it may be a good idea to have a second set of eyes on the changes.

Node 5.0-5.11
[abd101be1a] - http: disallow sending obviously invalid status codes (Brian White) #6291
[16b23b2c28] - http: skip body and next message of CONNECT res (Fedor Indutny) #6279
[a259ee4018] - http: unref socket timer on parser execute (Fedor Indutny) #6286
[c6ac6f2ea1] - http: Corrects IPv6 address in Host header (Mihai Potra) #5314
[63c601bc15] - http: speed up checkIsHttpToken (Jackson Tian) #4790
[5e6d706758] - src,http: fix uncaughtException miss in http (Trevor Norris) #5591
[b706b0c2c5] - http: remove old, confusing comment (Brian White) #5233
[ed36235248] - http: remove unnecessary check (Brian White) #5233
[7e82a566b3] - (SEMVER-MINOR) http: allow async createConnection() (Brian White) #4638
[411d813323] - http: do not emit upgrade on advertisement (Fedor Indutny) #4337
[a3b84a4c93] - (SEMVER-MINOR) http: strictly forbid invalid characters from headers (James M Snell)
[9b03af254a] - http: remove reference to onParserExecute (Tom Atkinson) #4773
[d755432fa9] - (SEMVER-MINOR) http: improves expect header handling (Daniel Sellers) #4501
[787c5d96bd] - http: remove variable redeclaration (Rich Trott) #4612
[1dd2d015d2] - (SEMVER-MINOR) http: handle errors on idle sockets (José F. Romaniello) #4482
[083ae166bb] - http: use self.keepAlive instead of self.options.keepAlive (Damian Schenkelman) #4407
[ffb4a6e0e4] - http: fix non-string header value concatenation (Brian White) #4460
[c77fd6829a] - (SEMVER-MINOR) http: 451 status code "Unavailable For Legal Reasons" (Max Barinov) #4377
[8f7af9a489] - http: remove excess calls to removeSocket (Dave) #4172
[b841967103] - http: Remove an unnecessary assignment (Bo Borgerson) #4323
[12e70fafd3] - http: fix pipeline regression (Fedor Indutny)
[a7f28a098e] - http: remove unneeded cb check from setTimeout() (Ashok Suthar) #3631
[ab03635fb1] - http: fix stalled pipeline bug (Fedor Indutny) #3342
[e655a437b3] - (SEMVER-MAJOR) http: do not allow multiple instances of certain response headers (James M Snell) #3090
[0094a8dad7] - (SEMVER-MAJOR) http: add callback is function check (James M Snell) #3090
[6192c9892f] - (SEMVER-MAJOR) http: add checkIsHttpToken check for header fields (James M Snell) 2526

Node 6.0-6.1
[5f76b24e5e] - (SEMVER-MAJOR) http: overridable clientError (Fedor Indutny) #4557

@dougwilson
Copy link
Contributor

@calvinmikael what are the changes between 5.x and 6.x? That's the biggest question here, because that does have breaking changes.

@ghost
Copy link
Author

ghost commented May 13, 2016

@dougwilson
Interestingly an appveyor test failed on Node 2.5.
Considering I may be going about this the wrong way by submitting this PR as an outside contributor re: blakeembrey/array-flatten#4 (comment) would you prefer if I left this update to the Express.js code team? It would be okay to decline this PR.

@LinusU
Copy link
Member

LinusU commented May 13, 2016

As for @LinusU, the reason they are pinned to the minor is to prioritize reproducible builds, at the expense of a very simple chore. There was a flurry of discussion about this prior to the TC and this pattern was the result. I think if we desire further discussion or re-evaluation, we just make a issue in the discussions repo or add it to TC agenda :)

Sounds good as it is too me 👍

@dougwilson
Copy link
Contributor

@calvinmikael, there is no issue with you doing it, sorry I mentioned that :) It was more about how that since usually it's not done that way, we just don't have documentation about how you would be expected to go about the process, so we just need to figure it out. I think maybe the best idea so far is the report idea?

@ghost
Copy link
Author

ghost commented May 13, 2016

@dougwilson
No problem. The report method is a good approach.

It may be considered that more time may be spent helping me complete all the requirements than a core team member going over the change themselves as a short chore if deemed necessary at all.

While I am able to test all the dependencies I don't feel that my knowledge of express qualifies me to determine which changes between Node versions are important to note.

@ghost ghost closed this May 14, 2016
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants