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

test: skip when openssl CLI doesn't exist #11095

Closed
wants to merge 1 commit into
from

Conversation

@sotayamashita
Member

sotayamashita commented Feb 1, 2017

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)

test

I fixed #9509.

@mscdex mscdex added the tls label Feb 1, 2017

@jasnell

jasnell approved these changes Feb 1, 2017

@sam-github

commit message is ungrammatical, change to

test: check openssl-cli exists

or

test: skip when openssl CLI doesn't exist

@sotayamashita

This comment has been minimized.

Show comment
Hide comment
@sotayamashita

sotayamashita Feb 1, 2017

Member

@sam-github I changed the commit message

Member

sotayamashita commented Feb 1, 2017

@sam-github I changed the commit message

@sotayamashita sotayamashita changed the title from test: check openssl-cli is exist to test: skip when openssl CLI doesn't exist Feb 1, 2017

@hiroppy

hiroppy approved these changes Feb 2, 2017

@hiroppy

This comment has been minimized.

Show comment
Hide comment
@sotayamashita

This comment has been minimized.

Show comment
Hide comment
@sotayamashita

sotayamashita Feb 2, 2017

Member

@abouthiroppy
I would appreciate if you could tell me how to pass the CI.
When I create a PR, should I run the CI ?

Member

sotayamashita commented Feb 2, 2017

@abouthiroppy
I would appreciate if you could tell me how to pass the CI.
When I create a PR, should I run the CI ?

@hiroppy

This comment has been minimized.

Show comment
Hide comment
@hiroppy

hiroppy Feb 2, 2017

Member

@sotayamashita I think these failures are not caused by code🙃 CC @nodejs/testing

Member

hiroppy commented Feb 2, 2017

@sotayamashita I think these failures are not caused by code🙃 CC @nodejs/testing

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Feb 2, 2017

Member

@sotayamashita those look like infrastructure problems.

CI 2: https://ci.nodejs.org/job/node-test-commit/7616/

Member

gibfahn commented Feb 2, 2017

@sotayamashita those look like infrastructure problems.

CI 2: https://ci.nodejs.org/job/node-test-commit/7616/

@sotayamashita

This comment has been minimized.

Show comment
Hide comment
Member

sotayamashita commented Feb 2, 2017

@gibfahn Thanks.

@sotayamashita

This comment has been minimized.

Show comment
Hide comment
@sotayamashita

sotayamashita Feb 2, 2017

Member

@sam-github I changed the commit message. Is it ok ?

Member

sotayamashita commented Feb 2, 2017

@sam-github I changed the commit message. Is it ok ?

@gibfahn

gibfahn approved these changes Feb 2, 2017

@joaocgreis

This comment has been minimized.

Show comment
Hide comment
@joaocgreis

joaocgreis Feb 2, 2017

Member

Aborted CI, only arm-fanned was pending because of nodejs/build#611

Member

joaocgreis commented Feb 2, 2017

Aborted CI, only arm-fanned was pending because of nodejs/build#611

@mhdawson

LGTM

@mhdawson

This comment has been minimized.

Show comment
Hide comment
Member

mhdawson commented Feb 3, 2017

requested changes have been made

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 7, 2017

Member

New CI since there's some red in that last run: https://ci.nodejs.org/job/node-test-pull-request/6273/

Member

jasnell commented Feb 7, 2017

New CI since there's some red in that last run: https://ci.nodejs.org/job/node-test-pull-request/6273/

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Feb 8, 2017

Member

@sotayamashita

When I create a PR, should I run the CI ?

Sorry, didn't see this before. Only collaborators have the access to run jobs in the Jenkins CI (ci.nodejs.org). Someone should run the CI for you when you submit the PR, if they don't you can comment asking for it to be run.

As long as you've followed CONTRIBUTING.md and run make -j4 test before submitting, that should be all you need to do.

Once CI has been run, you can click on the link and see the results, if there are any failures that seem related to changes you made you can investigate (but they could also just be infrastructure issues).

In general if you're not sure about something, you can always ask.

Member

gibfahn commented Feb 8, 2017

@sotayamashita

When I create a PR, should I run the CI ?

Sorry, didn't see this before. Only collaborators have the access to run jobs in the Jenkins CI (ci.nodejs.org). Someone should run the CI for you when you submit the PR, if they don't you can comment asking for it to be run.

As long as you've followed CONTRIBUTING.md and run make -j4 test before submitting, that should be all you need to do.

Once CI has been run, you can click on the link and see the results, if there are any failures that seem related to changes you made you can investigate (but they could also just be infrastructure issues).

In general if you're not sure about something, you can always ask.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Feb 13, 2017

Member

Landed in 5ffb7d7

Member

sam-github commented Feb 13, 2017

Landed in 5ffb7d7

sam-github added a commit that referenced this pull request Feb 13, 2017

test: skip when openssl CLI doesn't exist
PR-URL: #11095
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

@sam-github sam-github closed this Feb 13, 2017

@sotayamashita sotayamashita deleted the sotayamashita:feature/test-openssl-cli branch Feb 14, 2017

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 14, 2017

test: skip when openssl CLI doesn't exist
PR-URL: nodejs#11095
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 7, 2017

Member

needs a backport PR to land on v4 or v6

Member

jasnell commented Mar 7, 2017

needs a backport PR to land on v4 or v6

@sotayamashita

This comment has been minimized.

Show comment
Hide comment
@sotayamashita

sotayamashita Mar 8, 2017

Member

@jasnell I would like to implement but I don not know hot to do it. Is there document ?

Member

sotayamashita commented Mar 8, 2017

@jasnell I would like to implement but I don not know hot to do it. Is there document ?

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 8, 2017

Member

The process is fairly simple. To backport for v6, for instance, create a working branch off the v6-x-staging branch:

$ git checkout v6.x-staging
$ git checkout -b test-openssl-cli-v6-backport

Then cherry-pick the commit that landed in master

$ git cherry-pick 5ffb7d72bb6d8c827b66a337c3318e3ac9b3055e

You will see that there are a number of conflicts to be resolved. Make the necessary changes to fix those conflicts, then complete the cherry-pick using:

$ git add {the-files-that-were-changed}
$ git cherry-pick --continue

Push the branch to your fork on Github and open a new PR against v6.x-staging, indicating that this is a Backport of #11095.

Make sure to run make test and make lint.

Member

jasnell commented Mar 8, 2017

The process is fairly simple. To backport for v6, for instance, create a working branch off the v6-x-staging branch:

$ git checkout v6.x-staging
$ git checkout -b test-openssl-cli-v6-backport

Then cherry-pick the commit that landed in master

$ git cherry-pick 5ffb7d72bb6d8c827b66a337c3318e3ac9b3055e

You will see that there are a number of conflicts to be resolved. Make the necessary changes to fix those conflicts, then complete the cherry-pick using:

$ git add {the-files-that-were-changed}
$ git cherry-pick --continue

Push the branch to your fork on Github and open a new PR against v6.x-staging, indicating that this is a Backport of #11095.

Make sure to run make test and make lint.

@sotayamashita

This comment has been minimized.

Show comment
Hide comment
@sotayamashita

sotayamashita Mar 8, 2017

Member

@jasnell Thank you very much. I really appreciate your thoughtfulness.

Member

sotayamashita commented Mar 8, 2017

@jasnell Thank you very much. I really appreciate your thoughtfulness.

MylesBorins added a commit that referenced this pull request Apr 13, 2017

test: skip when openssl CLI doesn't exist
Backport-PR-URL: #12173
PR-URL: #11095
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

MylesBorins added a commit that referenced this pull request Apr 19, 2017

test: skip when openssl CLI doesn't exist
Backport-PR-URL: #12173
PR-URL: #11095
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

@MylesBorins MylesBorins referenced this pull request Apr 19, 2017

Merged

v6.10.3 proposal #12498

andrew749 added a commit to michielbaird/node that referenced this pull request Jul 19, 2017

test: skip when openssl CLI doesn't exist
Backport-PR-URL: nodejs/node#12173
PR-URL: nodejs/node#11095
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment