Skip to content

crypto: make LazyTransform compabile with Streams1#12380

Closed
mcollina wants to merge 1 commit intonodejs:masterfrom
mcollina:lazy-transform
Closed

crypto: make LazyTransform compabile with Streams1#12380
mcollina wants to merge 1 commit intonodejs:masterfrom
mcollina:lazy-transform

Conversation

@mcollina
Copy link
Member

@mcollina mcollina commented Apr 13, 2017

Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: #12269

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

crypto, streams

@mscdex mscdex added crypto Issues and PRs related to the crypto subsystem. stream Issues and PRs related to the stream subsystem. labels Apr 13, 2017
@sam-github
Copy link
Contributor

sam-github commented Apr 13, 2017

s/compabile/compatible/

cjihrig
cjihrig requested changes Apr 13, 2017
View reviewed changes
test/parallel/test-crypto-lazy-transform-writable.js Outdated
Copy link
Contributor

@cjihrig cjihrig Apr 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change this to // Calculate the expected result.

test/parallel/test-crypto-lazy-transform-writable.js Outdated
Copy link
Contributor

@cjihrig cjihrig Apr 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the third parameter should say the hash doesn't match.

Copy link
Contributor

@sam-github sam-github Apr 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just my two bits, but I think most description strings are a waste of time and space. The reported on failure line number leads directly to the assert code, which is quite readable. As-is, the string just repeats the arg name and the word "match", which is a synonym for "equal", so its just a comment that restates exactly what the code says, a classic anti-pattern.

@mcollina
crypto: make LazyTransform compabile with Streams1
06844ff 
Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: nodejs#12269
@mcollina
Copy link
Member Author

mcollina commented Apr 14, 2017

@cjihrig @sam-github updated.

CI: https://ci.nodejs.org/job/node-test-pull-request/7397/

@mcollina
Copy link
Member Author

mcollina commented Apr 14, 2017

Landed in 1ae172b

@mcollina mcollina closed this Apr 14, 2017
@mcollina mcollina deleted the lazy-transform branch April 14, 2017 14:17
mcollina added a commit that referenced this pull request Apr 14, 2017
@mcollina
crypto: make LazyTransform compabile with Streams1
1ae172b 
Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: #12269
PR-URL: #12380
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
evanlucas pushed a commit that referenced this pull request Apr 25, 2017
@mcollina @evanlucas
crypto: make LazyTransform compabile with Streams1
ebc69be 
Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: #12269
PR-URL: #12380
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@evanlucas evanlucas mentioned this pull request May 1, 2017
Merged
evanlucas pushed a commit that referenced this pull request May 1, 2017
@mcollina @evanlucas
crypto: make LazyTransform compabile with Streams1
18dbc1c 
Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: #12269
PR-URL: #12380
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
evanlucas pushed a commit that referenced this pull request May 2, 2017
@mcollina @evanlucas
crypto: make LazyTransform compabile with Streams1
8ac8e50 
Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: #12269
PR-URL: #12380
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins
Copy link
Contributor

MylesBorins commented May 15, 2017

@mcollina I've backported to v6.x-staging please lmk if you think this was a mistake and we can rebase it out.

MylesBorins pushed a commit that referenced this pull request May 15, 2017
@mcollina @MylesBorins
crypto: make LazyTransform compabile with Streams1
b1dc123 
Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: #12269
PR-URL: #12380
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins MylesBorins added land-on-v6.x notable-change PRs with changes that should be highlighted in changelogs. labels May 15, 2017
@mcollina
Copy link
Member Author

mcollina commented May 16, 2017

@MylesBorins you did well, it should be backported.

MylesBorins pushed a commit that referenced this pull request May 18, 2017
@mcollina @MylesBorins
crypto: make LazyTransform compabile with Streams1
0a79b2d 
Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: #12269
PR-URL: #12380
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins MylesBorins mentioned this pull request May 23, 2017
Merged
MylesBorins added a commit that referenced this pull request Jun 6, 2017
@MylesBorins
2017-06-06, Version 6.11.0 'Boron' (LTS)
40859e2 
This LTS release comes with 126 commits. This includes 40 which
are test related, 32 which are doc related, 12 which are
build / tool related and 4 commits which are updates to
dependencies.

Notable Changes:

* build:
  - support for building mips64el (nanxiongchao)
    #10991
* cluster:
  - disconnect() now returns a reference to the disconnected
    worker. (Sean Villars)
    #10019
* crypto:
  - ability to select cert store at runtime (Adam Majer)
    #8334
  - Use system CAs instead of using bundled ones (Adam Majer)
    #8334
  - The `Decipher` methods `setAuthTag()` and `setAAD` now return
    `this`. (Kirill Fomichev)
    #9398
  - adding support for OPENSSL_CONF again (Sam Roberts)
    #11006
  - make LazyTransform compabile with Streams1 (Matteo Collina)
    #12380
* deps:
  - upgrade libuv to 1.11.0 (cjihrig)
    #11094
  - upgrade libuv to 1.10.2 (cjihrig)
    #10717
  - upgrade libuv to 1.10.1 (cjihrig)
    #9647
  - upgrade libuv to 1.10.0 (cjihrig)
    #9267
* dns:
  - Implemented `{ttl: true}` for `resolve4()` and `resolve6()`
    (Ben Noordhuis)
    #9296
* process:
  - add NODE_NO_WARNINGS environment variable (cjihrig)
    #10842
* readline:
  - add option to stop duplicates in history (Danny Nemer)
    #2982
* src:
  - support "--" after "-e" as end-of-options (John Barboza)
    #10651
* tls:
  - new tls.TLSSocket() supports sec ctx options (Sam Roberts)
    #11005
  - Allow obvious key/passphrase combinations. (Sam Roberts)
    #10294

PR-URL: #13059
MylesBorins added a commit that referenced this pull request Jun 6, 2017
@MylesBorins
2017-06-06, Version 6.11.0 'Boron' (LTS)
4c5cbb7 
This LTS release comes with 126 commits. This includes 40 which
are test related, 32 which are doc related, 12 which are
build / tool related and 4 commits which are updates to
dependencies.

Notable Changes:

* build:
  - support for building mips64el (nanxiongchao)
    #10991
* cluster:
  - disconnect() now returns a reference to the disconnected
    worker. (Sean Villars)
    #10019
* crypto:
  - ability to select cert store at runtime (Adam Majer)
    #8334
  - Use system CAs instead of using bundled ones (Adam Majer)
    #8334
  - The `Decipher` methods `setAuthTag()` and `setAAD` now return
    `this`. (Kirill Fomichev)
    #9398
  - adding support for OPENSSL_CONF again (Sam Roberts)
    #11006
  - make LazyTransform compabile with Streams1 (Matteo Collina)
    #12380
* deps:
  - upgrade libuv to 1.11.0 (cjihrig)
    #11094
  - upgrade libuv to 1.10.2 (cjihrig)
    #10717
  - upgrade libuv to 1.10.1 (cjihrig)
    #9647
  - upgrade libuv to 1.10.0 (cjihrig)
    #9267
* dns:
  - Implemented `{ttl: true}` for `resolve4()` and `resolve6()`
    (Ben Noordhuis)
    #9296
* process:
  - add NODE_NO_WARNINGS environment variable (cjihrig)
    #10842
* readline:
  - add option to stop duplicates in history (Danny Nemer)
    #2982
* src:
  - support "--" after "-e" as end-of-options (John Barboza)
    #10651
* tls:
  - new tls.TLSSocket() supports sec ctx options (Sam Roberts)
    #11005
  - Allow obvious key/passphrase combinations. (Sam Roberts)
    #10294

PR-URL: #13059
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@mcollina @andrew749
crypto: make LazyTransform compabile with Streams1
4980c49 
Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: nodejs/node#12269
PR-URL: nodejs/node#12380
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@MylesBorins @andrew749
2017-06-06, Version 6.11.0 'Boron' (LTS)
e5cdc26 
This LTS release comes with 126 commits. This includes 40 which
are test related, 32 which are doc related, 12 which are
build / tool related and 4 commits which are updates to
dependencies.

Notable Changes:

* build:
  - support for building mips64el (nanxiongchao)
    nodejs/node#10991
* cluster:
  - disconnect() now returns a reference to the disconnected
    worker. (Sean Villars)
    nodejs/node#10019
* crypto:
  - ability to select cert store at runtime (Adam Majer)
    nodejs/node#8334
  - Use system CAs instead of using bundled ones (Adam Majer)
    nodejs/node#8334
  - The `Decipher` methods `setAuthTag()` and `setAAD` now return
    `this`. (Kirill Fomichev)
    nodejs/node#9398
  - adding support for OPENSSL_CONF again (Sam Roberts)
    nodejs/node#11006
  - make LazyTransform compabile with Streams1 (Matteo Collina)
    nodejs/node#12380
* deps:
  - upgrade libuv to 1.11.0 (cjihrig)
    nodejs/node#11094
  - upgrade libuv to 1.10.2 (cjihrig)
    nodejs/node#10717
  - upgrade libuv to 1.10.1 (cjihrig)
    nodejs/node#9647
  - upgrade libuv to 1.10.0 (cjihrig)
    nodejs/node#9267
* dns:
  - Implemented `{ttl: true}` for `resolve4()` and `resolve6()`
    (Ben Noordhuis)
    nodejs/node#9296
* process:
  - add NODE_NO_WARNINGS environment variable (cjihrig)
    nodejs/node#10842
* readline:
  - add option to stop duplicates in history (Danny Nemer)
    nodejs/node#2982
* src:
  - support "--" after "-e" as end-of-options (John Barboza)
    nodejs/node#10651
* tls:
  - new tls.TLSSocket() supports sec ctx options (Sam Roberts)
    nodejs/node#11005
  - Allow obvious key/passphrase combinations. (Sam Roberts)
    nodejs/node#10294

PR-URL: nodejs/node#13059
@akaila akaila mentioned this pull request Oct 19, 2017
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

+1 more reviewer

@sam-github sam-github sam-github left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

crypto Issues and PRs related to the crypto subsystem. notable-change PRs with changes that should be highlighted in changelogs. stream Issues and PRs related to the stream subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@mcollina @sam-github @MylesBorins @jasnell @addaleax @cjihrig @mscdex