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

Make bind parameter take the actual interface, and port the port #121

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

tevino
Copy link
Contributor

@tevino tevino commented Mar 30, 2017

No description provided.

badboy added 2 commits March 29, 2017 14:20
BREAKING CHANGE: This renames the old `bind` configuration option
                 to `port` and introduces a new configuration option
                 `bind` that now takes a single IP to listen on.
@tevino
Copy link
Contributor Author

tevino commented Mar 30, 2017

@badboy corvus.conf should be changed as well, it's used for testing, thus the failure.

@badboy
Copy link
Contributor

badboy commented Mar 30, 2017

Changed corvus.conf.
Keep in mind this would be a breaking change in corvus and would affect all users. I grepped through the code to find all places of config.bind usage, but I might have missed some.

@badboy
Copy link
Contributor

badboy commented Mar 30, 2017

In addition: this includes the minimal "get ipv6 working" patch. I did a manual test, but it would need to be tested more before getting merged.

@badboy
Copy link
Contributor

badboy commented Mar 30, 2017

I fixed the test now, but it's now lacking any tests for the new bind option. I will add that later.

@tevino
Copy link
Contributor Author

tevino commented Mar 31, 2017

For our production case, we get no benefits but rather extra work(config file compatibility) by merging this, but I think this is the right thing to do, so we'll merge this later on, next release I hope.

You could make changes until then.

@badboy
Copy link
Contributor

badboy commented Mar 31, 2017

I do understand the friction it will cause due to config file changes. I'd like to test this a bit further, including proper tests and IPv6 support.
Happy to work on this for the next release

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@badboy
Copy link
Contributor

badboy commented Feb 24, 2023

lol nope.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants