-
Notifications
You must be signed in to change notification settings - Fork 923
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
feat: add HOST/--host option #1335
Conversation
a4866cd
to
6e6a410
Compare
Speaking of (broken) tests, I'm trying to fix them. I've also been thinking about how to test this new feature and I don't believe adding an integration test to actually check network interface setup is what we want: ensuring that the expected values are passed down to Node |
testing that the |
It turns out fixing the tests is probably 10x the effort of implementing the feature... it might take some time |
Other than the IPv6 issue I referenced in my PR I'd like to propose to rename the config variables from |
I also prefer @gr2m what do you say? |
I have not added any new tests, I just fixed the old ones. During this process I noticed, that for some reason the listen() behavior changes when 0.0.0.0 (the previous default value) was used. I still dont know why this happens, but found another issue with the default value: Missing Ipv6 support. I removed the state properties and listen() is now not called with a second argument when bindAddress is undefined.
b291c51
to
38f28b7
Compare
Great points, thank you! Let's use |
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.
looks mostly good, just a few nits
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 looks great to me, thank you @shaftoe 💐 Are you on your side?
I meant to say "Are you ready on your side?", sorry for the typo. Englisch ist auch nicht meine erste Sprache, kann die Missverständnisse gut nachvollziehen ;) |
Yes, definitely, please go ahead
Alles klar! Aber Italienisch ist meine Muttersprache ;) |
🎉 This PR is included in version 10.4.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Tentative implementation of
--bind-address
option to bind Probot to a specific IP address and tohttp://localhost:3000/
by default.Suggested by @gr2m in this other issue: #1329 (comment)
Comments inline the patch about doubts and/or implementation details.
Test still missing, will work on that asap
View rendered docs/configuration.md