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.

Sign up for GitHub

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

Jump to bottom

v4 backport of pr 5463 #11002

Closed
wants to merge 2 commits into from
Closed

v4 backport of pr 5463 #11002

wants to merge 2 commits into from

Conversation

sam-github
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. v4.x labels Jan 25, 2017
@sam-github sam-github added lts-watch-v4.x and removed c++ Issues and PRs that require attention from people who are familiar with C++. v4.x labels Jan 25, 2017
@sam-github sam-github mentioned this pull request Jan 25, 2017
Merged
@mscdex mscdex added v4.x c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations. and removed lts-watch-v4.x labels Jan 25, 2017
@MylesBorins
Copy link
Contributor

@sam-github this needs a rebase

@sam-github
Copy link
Contributor Author

@MylesBorins done

@sam-github
Copy link
Contributor Author

@MylesBorins anything else you need here?

@MylesBorins MylesBorins added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 21, 2017
@MylesBorins
Copy link
Contributor

@sam-github due to this being semver-minor it will require approval by the LTS working group. I've tagged it accordingly to be brought up in the next meeting

@sam-github
Copy link
Contributor Author

thanks, @MylesBorins

@jasnell
Copy link
Member

jasnell commented Feb 22, 2017

+1 for landing in LTS

sam-github and others added 2 commits February 22, 2017 09:52
@sam-github
constants: define ENGINE_METHOD_RSA
6f92c50 
constants.ENGINE_METHOD_RSA was documented, but not implemented.

PR-URL: nodejs#5463
Reviewed-By: Fedor Indutny <[email protected]>
@sam-github
doc: correct name of engine methods
ff68a44 
ENGINE_METHOD_PKEY_METH and ENGINE_METHOD_PKEY_ASN1_METH are misspelled
in the documentation, both should be ..._METHS.

PR-URL: nodejs#5463
Reviewed-By: Fedor Indutny <[email protected]>
@MylesBorins
Copy link
Contributor

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

jasnell pushed a commit that referenced this pull request Mar 3, 2017
@sam-github @jasnell
constants: define ENGINE_METHOD_RSA
94964d0 
Original Commit Message:
  constants.ENGINE_METHOD_RSA was documented, but not implemented.

  PR-URL: #5463
  Reviewed-By: Fedor Indutny <[email protected]>

Backport-Of: #5463
PR-URL: #11002
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this pull request Mar 3, 2017
@sam-github @jasnell
doc: correct name of engine methods
98d6b3f 
Original Commit Message:
  ENGINE_METHOD_PKEY_METH and ENGINE_METHOD_PKEY_ASN1_METH are misspelled
  in the documentation, both should be ..._METHS.

  PR-URL: #5463
  Reviewed-By: Fedor Indutny <[email protected]>

Backport-Of: #5463
PR-URL: #11002
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Mar 3, 2017

Landed in 94964d0...98d6b3f

@jasnell jasnell closed this Mar 3, 2017
@sam-github sam-github deleted the backport-5463-to-v4 branch March 3, 2017 15:28
MylesBorins pushed a commit that referenced this pull request Mar 21, 2017
@sam-github @MylesBorins
constants: define ENGINE_METHOD_RSA
e53154a 
Original Commit Message:
  constants.ENGINE_METHOD_RSA was documented, but not implemented.

  PR-URL: #5463
  Reviewed-By: Fedor Indutny <[email protected]>

Backport-Of: #5463
PR-URL: #11002
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 21, 2017
@sam-github @MylesBorins
doc: correct name of engine methods
8087825 
Original Commit Message:
  ENGINE_METHOD_PKEY_METH and ENGINE_METHOD_PKEY_ASN1_METH are misspelled
  in the documentation, both should be ..._METHS.

  PR-URL: #5463
  Reviewed-By: Fedor Indutny <[email protected]>

Backport-Of: #5463
PR-URL: #11002
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 22, 2017
@sam-github @MylesBorins
constants: define ENGINE_METHOD_RSA
7ca1314 
Original Commit Message:
  constants.ENGINE_METHOD_RSA was documented, but not implemented.

  PR-URL: #5463
  Reviewed-By: Fedor Indutny <[email protected]>

Backport-Of: #5463
PR-URL: #11002
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 22, 2017
@sam-github @MylesBorins
doc: correct name of engine methods
5af65b2 
Original Commit Message:
  ENGINE_METHOD_PKEY_METH and ENGINE_METHOD_PKEY_ASN1_METH are misspelled
  in the documentation, both should be ..._METHS.

  PR-URL: #5463
  Reviewed-By: Fedor Indutny <[email protected]>

Backport-Of: #5463
PR-URL: #11002
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 4, 2017
@sam-github @MylesBorins
constants: define ENGINE_METHOD_RSA
951121f 
Original Commit Message:
  constants.ENGINE_METHOD_RSA was documented, but not implemented.

  PR-URL: #5463
  Reviewed-By: Fedor Indutny <[email protected]>

Backport-Of: #5463
PR-URL: #11002
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 4, 2017
@sam-github @MylesBorins
doc: correct name of engine methods
6ac53f1 
Original Commit Message:
  ENGINE_METHOD_PKEY_METH and ENGINE_METHOD_PKEY_ASN1_METH are misspelled
  in the documentation, both should be ..._METHS.

  PR-URL: #5463
  Reviewed-By: Fedor Indutny <[email protected]>

Backport-Of: #5463
PR-URL: #11002
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins
Copy link
Contributor

@sam-github I've backed this out of v4.x staging as I do not believe we will be doing another semver minor release.

I am unable to reopen this PR so LMK what you think we should do

@sam-github
Copy link
Contributor Author

@gibfahn what do you think, how troublesome will not haveing env var control of FIPS be for us? Should we advocate for/offer to do another semver-minor on v4?

Also, didn't this feature use to exist on v4, but got lost, and this just readds it? So it could be described as a bug-fix? ;-)

@MylesBorins
Copy link
Contributor

@sam-github if it could land as a bug fix we could potentially land it in a patch. This should come up at LTS WG

@gibfahn
Copy link
Member

gibfahn commented Apr 18, 2017

how troublesome will not haveing env var control of FIPS be for us?

@sam-github is that affected by this PR? In general FIPS is on by default in v4.x, so that should be fine. The issue is people wanting to make sure it's turned on by default in v6.x.

I'll add it to the lts issue to make sure we cover it anyway.

@gibfahn gibfahn mentioned this pull request Apr 18, 2017
Closed
31 tasks
@sam-github
Copy link
Contributor Author

Sorry, thought this was the OPENSSL_CONF PR, :shamefaced:

I think we should abandon it. Clearly noone is using the engine, or a bug would have been reported. The second commit, the doc one, could apply to 4.x, but I haven't had the energy to backport docs to 4.x.

Marking "don't land"

@sam-github
Copy link
Contributor Author

Its not clear to me how seriously we take the docs. When the docs say it works one way, but the code does another, changing the code would be semver-major. But when the docs say a feature exists, and it doesn't, when we add the feature, is it minor or patch?

This is a case of a claimed feature that didn't exist, but since no one cares, and I only found it during review of the docs/code, I don't want to stress about it unless someone actually complains. Engines aren't used very often.

Trott pushed a commit to Trott/io.js that referenced this pull request Aug 1, 2017
@sam-github @Trott
constants: define ENGINE_METHOD_RSA
9e97dbe 
Original Commit Message:
  constants.ENGINE_METHOD_RSA was documented, but not implemented.

  PR-URL: nodejs#5463
  Reviewed-By: Fedor Indutny <[email protected]>

Backport-Of: nodejs#5463
PR-URL: nodejs#11002
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Trott pushed a commit to Trott/io.js that referenced this pull request Aug 1, 2017
@sam-github @Trott
doc: correct name of engine methods
0cd4c0d 
Original Commit Message:
  ENGINE_METHOD_PKEY_METH and ENGINE_METHOD_PKEY_ASN1_METH are misspelled
  in the documentation, both should be ..._METHS.

  PR-URL: nodejs#5463
  Reviewed-By: Fedor Indutny <[email protected]>

Backport-Of: nodejs#5463
PR-URL: nodejs#11002
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@sam-github @MylesBorins @jasnell @gibfahn @addaleax @mscdex @nodejs-github-bot