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

fix(cli): force test host to be HOST env var or ipv4 interface #2433

Merged
merged 1 commit into from Feb 20, 2019

Conversation

Projects
None yet
6 participants
@raymondfeng
Copy link
Member

raymondfeng commented Feb 20, 2019

Fixes #1995

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated
  • Agree to the CLA (Contributor License Agreement) by clicking and signing

@raymondfeng raymondfeng requested a review from bajtos as a code owner Feb 20, 2019

@raymondfeng raymondfeng force-pushed the fix-1995 branch from 74b27e7 to 7dc82ef Feb 20, 2019

@dhmlau

dhmlau approved these changes Feb 20, 2019

@raymondfeng raymondfeng force-pushed the fix-1995 branch from 7dc82ef to 9230590 Feb 20, 2019

@b-admike
Copy link
Member

b-admike left a comment

LGTM; hopefully this is a happy default value for most use cases.

@raymondfeng raymondfeng merged commit 1664d4f into master Feb 20, 2019

4 checks passed

clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 90.474%
Details

@b-admike b-admike deleted the fix-1995 branch Feb 21, 2019

@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Feb 26, 2019

@raymondfeng what is the reasoning for adding this new logic to the project template, instead of modifying givenHttpServerConfig directly? givenHttpServerConfig is a test helper provided by @loopback/testlab, it's meant to be use in tests.

We are already detecting Travis CI and enforcing IPv4 there, it would make a lot of sense to me to detect other Docker environments too.

const hostConfig = process.env.TRAVIS ? {host: '127.0.0.1'} : {};

@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Feb 26, 2019

I had thought about detecting Docker in givenHttpServerConfig() but the logic is not that simple. Docker container by default has ipv6 disabled but it's enabled. Adding the logic in the template makes it easy to customize for user projects.

@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Feb 28, 2019

I see. I am proposing to decouple the Docker fix from the config customization, please review #2502

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.