-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Adding listen address option so that IPv6 and loopback interfaces can be used. #2479
Adding listen address option so that IPv6 and loopback interfaces can be used. #2479
Conversation
…work correctly with it reference: karma-runner#2477
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
I signed the cla. Not exactly sure how this works. Is that all I have to do? |
CLAs look good, thanks! |
LGTM |
if (typeof (arg2) === 'function') { | ||
callback = arg2 | ||
} else | ||
if (typeof (arg3) === 'function') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if
can probably be moved up a line so that it reads else if (...) {
, which is more standard
@grifball could you squash commits in to one Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should influence the value of config.hostname
, because now that this can be changed to any value, config.hostname
going to localhost
can fail.
I started writing something that automatically tries to set hostname when listenAddress is set but hostname isn't, but it seems out of scope. Figuring out which IP to use for hostname based on the value of listenAddress is really difficult, and can even be impossible in more complex interface setups. |
Let me know what you think of this solution. I'm not sure how to change the label on this PR. |
testConfig.set(cliOptions) | ||
} | ||
if (testConfig.hostname == null && testConfig.listenAddress != null) { | ||
log.warn('ListenAddress is set but hostname isn\'t. If your browsers fail ' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to print the current values of both the variables here.
testConfig.hostname = null | ||
testConfig.listenAddress = null | ||
if (configModule != null) { | ||
configModule(testConfig) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not do this, as this means we are evaluating the code from the user twice. Best case this slows things down, worst case it has unexpected side effects. You can add the check probably around the original invocition of the configModule
Thank you, yes I think it's fine to just warn the user about this. Left some comments about the implementation. |
Alright, I implemented those changes. Let me know if anything else needs changing. |
Do we want weak equality ( |
This should be a safe enough check. Unless they're setting these values to null and expecting the server to work correctly, which explicit string checks wouldn't solve anyway. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great, but I just realised there are not really tests covering this. Could you add some please?
I added a simple test to make sure that listen address was called with the configured listenAddress. That should cover all the functionality for this PR. |
Thank you |
This docs update was missing in the karma-runner#2479 that added option for configurable listening address. Thanks!
This docs update was missing in the #2479 that added option for configurable listening address.
# [5.0.0](v4.4.1...v5.0.0) (2020-04-09) ### Bug Fixes * install semantic-release as a regular dev dependency ([#3455](#3455)) ([1eaf35e](1eaf35e)) * **ci:** echo travis env that gates release after_success ([#3446](#3446)) ([b8b2ed8](b8b2ed8)) * **ci:** poll every 10s to avoid rate limit. ([#3388](#3388)) ([91e7e00](91e7e00)) * **middleware/runner:** handle file list rejections ([#3400](#3400)) ([80febfb](80febfb)), closes [#3396](#3396) [#3396](#3396) * **server:** cleanup import of the removed method ([#3439](#3439)) ([cb1bcbf](cb1bcbf)) * **server:** createPreprocessor was removed ([#3435](#3435)) ([5c334f5](5c334f5)) * **server:** detection new MS Edge Chromium ([#3440](#3440)) ([7166ce2](7166ce2)) * **server:** replace optimist on yargs lib ([#3451](#3451)) ([ec1e69a](ec1e69a)), closes [#2473](#2473) * **server:** Report original error message ([#3415](#3415)) ([79ee331](79ee331)), closes [#3414](#3414) ### Code Refactoring * use native Promise instead of Bluebird ([#3436](#3436)) ([33a069f](33a069f)), closes [/github.com//pull/3060#discussion_r284797390](https://github.com//github.com/karma-runner/karma/pull/3060/issues/discussion_r284797390) ### Continuous Integration * drop node 8, adopt node 12 ([#3430](#3430)) ([a673aa8](a673aa8)) ### Features * **docs:** document `DEFAULT_LISTEN_ADDR` constant ([#3443](#3443)) ([057d527](057d527)), closes [#2479](#2479) * **karma-server:** added log to the server.js for uncaught exception ([#3399](#3399)) ([adc6a66](adc6a66)) * **preprocessor:** obey Pattern.isBinary when set ([#3422](#3422)) ([708ae13](708ae13)), closes [#3405](#3405) ### BREAKING CHANGES * Karma plugins which rely on the fact that Karma uses Bluebird promises may break as Bluebird-specific API is no longer available on Promises returned by the Karma core * **server:** Deprecated createPreprocessor removed, karma-browserify < 7 version doesn't work * no more testing on node 8.
This docs update was missing in the karma-runner#2479 that added option for configurable listening address.
# [5.0.0](karma-runner/karma@v4.4.1...v5.0.0) (2020-04-09) ### Bug Fixes * install semantic-release as a regular dev dependency ([karma-runner#3455](karma-runner#3455)) ([1eaf35e](karma-runner@1eaf35e)) * **ci:** echo travis env that gates release after_success ([karma-runner#3446](karma-runner#3446)) ([b8b2ed8](karma-runner@b8b2ed8)) * **ci:** poll every 10s to avoid rate limit. ([karma-runner#3388](karma-runner#3388)) ([91e7e00](karma-runner@91e7e00)) * **middleware/runner:** handle file list rejections ([karma-runner#3400](karma-runner#3400)) ([80febfb](karma-runner@80febfb)), closes [karma-runner#3396](karma-runner#3396) [karma-runner#3396](karma-runner#3396) * **server:** cleanup import of the removed method ([karma-runner#3439](karma-runner#3439)) ([cb1bcbf](karma-runner@cb1bcbf)) * **server:** createPreprocessor was removed ([karma-runner#3435](karma-runner#3435)) ([5c334f5](karma-runner@5c334f5)) * **server:** detection new MS Edge Chromium ([karma-runner#3440](karma-runner#3440)) ([7166ce2](karma-runner@7166ce2)) * **server:** replace optimist on yargs lib ([karma-runner#3451](karma-runner#3451)) ([ec1e69a](karma-runner@ec1e69a)), closes [karma-runner#2473](karma-runner#2473) * **server:** Report original error message ([karma-runner#3415](karma-runner#3415)) ([79ee331](karma-runner@79ee331)), closes [karma-runner#3414](karma-runner#3414) ### Code Refactoring * use native Promise instead of Bluebird ([karma-runner#3436](karma-runner#3436)) ([33a069f](karma-runner@33a069f)), closes [/github.com/karma-runner/pull/3060#discussion_r284797390](https://github.com//github.com/karma-runner/karma/pull/3060/issues/discussion_r284797390) ### Continuous Integration * drop node 8, adopt node 12 ([karma-runner#3430](karma-runner#3430)) ([a673aa8](karma-runner@a673aa8)) ### Features * **docs:** document `DEFAULT_LISTEN_ADDR` constant ([karma-runner#3443](karma-runner#3443)) ([057d527](karma-runner@057d527)), closes [karma-runner#2479](karma-runner#2479) * **karma-server:** added log to the server.js for uncaught exception ([karma-runner#3399](karma-runner#3399)) ([adc6a66](karma-runner@adc6a66)) * **preprocessor:** obey Pattern.isBinary when set ([karma-runner#3422](karma-runner#3422)) ([708ae13](karma-runner@708ae13)), closes [karma-runner#3405](karma-runner#3405) ### BREAKING CHANGES * Karma plugins which rely on the fact that Karma uses Bluebird promises may break as Bluebird-specific API is no longer available on Promises returned by the Karma core * **server:** Deprecated createPreprocessor removed, karma-browserify < 7 version doesn't work * no more testing on node 8.
reference: #2477