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

Extend NetUtils for network range scanning #4375

Merged
merged 8 commits into from
Oct 7, 2024

Conversation

lsiepel
Copy link
Contributor

@lsiepel lsiepel commented Sep 12, 2024

Multiple bindings scan the network for devices by looping though all ip addresses. When trying to get rid of the apache libs, i also noticed the code is similar and it would be better to deduplicate this.

In future we can discuss if this scanning should be limited to the openHAB network setting or to all interfaces, but that is beyond the scope of this PR. Atleast we then have a central place to control that behaviour.

Reference: #3965

Signed-off-by: Leo Siepel <[email protected]>
…tUtil.java

Co-authored-by: Holger Friedrich <[email protected]>
Signed-off-by: lsiepel <[email protected]>
@lsiepel
Copy link
Contributor Author

lsiepel commented Sep 29, 2024

Would be nice to have this in the next milestone. Anything I can do to get this forward?

Copy link
Member

@holgerfriedrich holgerfriedrich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @lsiepel, it seems we have some doubts when it comes to the network scanning - we had some trouble with docker VMs during the last release phase. I am still not sure how this scanning should be implemented, restricting to primary interface, etc....

Additionally, it is not clear to me what is the benefit of getting rid of apache commons in this area - especially as we still need it for the telnet based addons.

Maybe you could try to convince us ;-)
Overall, the methods below do not harm in core. But not sure if this will be the basis for all plugins which try some kind of scanning or if we need to elaborate a more complex concept.

Besides that, one technical comment below.

@lsiepel
Copy link
Contributor Author

lsiepel commented Oct 5, 2024

You write we, so I suppose there has been a discussion amongst core maintainers?

  1. With or without this PR, network scanning is happening. I would rather have a central place to influence this behaviour, then rely on multiple binding implementations.
  2. This de-duplicates code
  3. It removes/reduces the dependency on apache. In general less dependency is good. There might be other issues that prevent full removal of apache, but I have always learned to make problems smaller and take them one by one. We have already come a long way. Please check Removal of dependency on 'org.apache.commons.*' openhab-addons#7722
  4. It does not harm core in a negative way.

If there are still doubts, i’m happy to incorporate suggestions as I really tho k this is a step forward.

@holgerfriedrich
Copy link
Member

You write we, so I suppose there has been a discussion amongst core maintainers?

Sorry, you got me wrong. No recent discussion about this, my comment is just based on what I remember from the discussion about IP "scanning" for addon detection.

1. With or without this PR, network scanning is happening.  I would rather have a central place to influence this behaviour, then rely on multiple binding implementations.

2. This de-duplicates code

3. It removes/reduces the dependency on apache. In general less dependency is good. There might be other issues that prevent full removal of apache, but I have always learned to make problems smaller and take them one by one.  We have already come a long way. Please check [Removal of dependency on 'org.apache.commons.*' openhab-addons#7722](https://github.com/openhab/openhab-addons/issues/7722)

4. It does not harm core in a negative way.

If there are still doubts, i’m happy to incorporate suggestions as I really tho k this is a step forward.

Fair enough, lets go forward and get this into core. 👍

@lsiepel
Copy link
Contributor Author

lsiepel commented Oct 5, 2024

Thanks for looking into this and sharing your doubts.

Copy link
Member

@holgerfriedrich holgerfriedrich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @lsiepel for the patience regarding this PR.

I now went through the code and also had a quick look into the PRs in addons - I can now understand why you want this in core.

I have some doubts regarding that gist you added for the helper functions. See my comment below.

@holgerfriedrich holgerfriedrich added the enhancement An enhancement or new feature of the Core label Oct 6, 2024
Signed-off-by: Leo Siepel <[email protected]>
Signed-off-by: Leo Siepel <[email protected]>
Signed-off-by: Leo Siepel <[email protected]>
Copy link
Member

@holgerfriedrich holgerfriedrich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Thanks for taking all the comments into consideration.
Looks much simpler now 👍

@holgerfriedrich holgerfriedrich merged commit d9e5df0 into openhab:main Oct 7, 2024
5 checks passed
@holgerfriedrich holgerfriedrich added this to the 4.3 milestone Oct 7, 2024
@lsiepel lsiepel deleted the netutils-extend branch October 7, 2024 21:59
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

Successfully merging this pull request may close these issues.

2 participants