-
-
Notifications
You must be signed in to change notification settings - Fork 428
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
IpAddonFinder: Skip installed addons #4013
Conversation
Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>
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.
Otherwise LGTM
...scovery.addon.ip/src/main/java/org/openhab/core/config/discovery/addon/ip/IpAddonFinder.java
Outdated
Show resolved
Hide resolved
...scovery.addon.ip/src/main/java/org/openhab/core/config/discovery/addon/ip/IpAddonFinder.java
Outdated
Show resolved
Hide resolved
...scovery.addon.ip/src/main/java/org/openhab/core/config/discovery/addon/ip/IpAddonFinder.java
Outdated
Show resolved
Hide resolved
...scovery.addon.ip/src/main/java/org/openhab/core/config/discovery/addon/ip/IpAddonFinder.java
Outdated
Show resolved
Hide resolved
@@ -356,4 +364,14 @@ public Set<AddonInfo> getSuggestedAddons() { | |||
public String getServiceName() { | |||
return SERVICE_NAME; | |||
} | |||
|
|||
private boolean isAddonInstalled(String addonId) { |
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.
Maybe we should move this to BaseAddonFinder. It would allow to use this in other add-on finders as well.
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.
Sure, if it is needed, I can do so. I had the impression, that only IP finder will skip searching if an addon is already installed.
Should we move it, although it introduces some overhead to the other finders? OSGI would then call register all addonservices to BaseAddonFinder using
@Reference(cardinality = ReferenceCardinality.MULTIPLE, policy = ReferencePolicy.DYNAMIC) protected void addAddonService(AddonService featureService) {
...
If we want to share it, maybe extending AddonResource would be an option.
@wborn Maybe you could comment on this as well....
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.
If we want to share it, maybe extending AddonResource would be an option.
The AddonResource
is in the org.openhab.core.io.rest.core bundle which is not the type of bundle you want other bundles to depend on. That's because that bundle depends on JAX-RS, Swagger etc.
It might be better to add something reusable to the org.openhab.core.addon bundle. It already provides the AddonService
interface so anything that needs to interact with AddonService
s already depends on it. E.g. an AddonServiceRegistry
could be added there that makes the logic of the AddonResource
more reusable. The AddonResource
could then also use it.
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.
Ok, good to know. Actually, an AddonServiceRegistry
was the first thing I was looking for - and wondered that it is not yet there.
However, hhis is clearly out of scope of this PR.
Can we move forward with this one as it is for now?
Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>
196962e
to
80646fa
Compare
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.
Thanks for the improvement!
Signed-off-by: Holger Friedrich <mail@holger-friedrich.de> Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
Query AddonService to check if an addon is already installed. In case, skip scanning for the addon.
Fixes #3967