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

Add alternative utility class for apache subnetutils #3965

Open
lsiepel opened this issue Dec 27, 2023 · 6 comments
Open

Add alternative utility class for apache subnetutils #3965

lsiepel opened this issue Dec 27, 2023 · 6 comments
Labels
enhancement An enhancement or new feature of the Core

Comments

@lsiepel
Copy link
Contributor

lsiepel commented Dec 27, 2023

We try to get rid of dependencies to apache libs per openhab/openhab-addons#7722

we made great progress with #3738
Now there are 5 bindings that make use of subnetutils. I suggest a similar approach here, I can create an alternative util class in core to prevent code duplication. will be based on openhab/openhab-addons#14430 (comment) where the implementation was confirmed working as expected.

@openhab/core-maintainers any thoughts before I start working on it?

@lsiepel lsiepel added the enhancement An enhancement or new feature of the Core label Dec 27, 2023
@J-N-K
Copy link
Member

J-N-K commented Dec 28, 2023

I don't think that's necessary. We need org.apache.commons.net anyway, so we can also use it in add-ons.

@lsiepel
Copy link
Contributor Author

lsiepel commented Dec 28, 2023

Oké, in that case I think openhab/openhab-addons#7722 can be closed, as these are the last dependencies in addons repo.
Full list is here openhab/openhab-addons#7722 (comment)

@wborn wdyt?

@wborn
Copy link
Member

wborn commented Dec 29, 2023

Maybe you can use NetUtil / NetworkAddressService instead? It has a getAllInterfaceAddresses method.
Add-ons should also respect the openHAB "Network Configuration" settings like not using IPv6 if it is disabled.
Currently we cannot remove commons-net because it is a dependency required for rfc2217 connections using nrjavaserial.

@lsiepel
Copy link
Contributor Author

lsiepel commented Dec 29, 2023

This issue is not about removing commons-net at this moment, its scope is much smaller. It is about reducing some dependencies to commons-net. I know we still depend on commons-net due to nrjavaserial, but why not reduce it as much as possible?

I also see this as an opportunity to reduce and improve code that is used in multiple bindings that have about the same functionality. For instance, when i do a quick scan, it looks like none of the 5 involved bindings respect the openHAB "Network Configuration" settings.

@wborn
Copy link
Member

wborn commented Dec 29, 2023

I also see this as an opportunity to reduce and improve code that is used in multiple bindings that have about the same functionality. For instance, when i do a quick scan, it looks like none of the 5 involved bindings respect the openHAB "Network Configuration" settings.

Yes that might be a nice improvement to fix multiple issues at once. 👍

@lsiepel
Copy link
Contributor Author

lsiepel commented Sep 12, 2024

While working on this, i doubt if 'respecting' the openHAB "Network Configuration" settings is an improvement.

This configuration setting lets you choose a 'Primary Address'. How should one interpret the "Network Configuration" setting?

My initial thought was that bindings should only setup connections using this address. But when looking at multiple bindings i think this setting is much more limited. Main concerns when limiting bindings to this settings:

  • none of the bindings respect this (breaking changes)
  • what if you have vlan's or any other network segmentation with different devices on differnet networks.
  • waht is the purpose of limiting bindings to use a network

So i guess this setting is only usefull for addons that expose a service and need to bind to a network address (like homekit etc).

TL:DR i think if the related PR's are merged, this issue can also be closed. WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature of the Core
Projects
None yet
Development

No branches or pull requests

3 participants