Improved the feature installation logic #256
Conversation
- wait for config update when online status has changed - removed static methods and logger - proper handling of "minimal" feature - introduced a sync job that checks if everything is installed as expected (which serves as a retry mechanism in case of failed downloads) fixes openhab/openhab-distro#602 Signed-off-by: Kai Kreuzer <kai@openhab.org>
There was a problem hiding this comment.
Thanks for your contribution ;-) I have added some minor comments.
Btw what is the main change which actually fixes openhab/openhab-distro#602
| @@ -106,14 +116,34 @@ protected void activate(final Map<String, Object> config) { | |||
| setOnlineRepoUrl(); | |||
| setLegacyRepoUrl(); | |||
| modified(config); | |||
| scheduler = Executors.newSingleThreadScheduledExecutor(); | |||
There was a problem hiding this comment.
I would maybe initialize where I declare it, for me that feels safer.
There was a problem hiding this comment.
No, because it needs to be (re-)initialized upon activation and shut down on deactivation.
| logger.debug("Running scheduled sync job"); | ||
| try { | ||
| Dictionary<String, Object> cfg = configurationAdmin.getConfiguration(ADDONS_PID).getProperties(); | ||
| Iterator<String> keysIter = Iterators.forEnumeration(cfg.keys()); |
There was a problem hiding this comment.
:-( unfortunately this is the best way which I can also find..
| @@ -302,11 +332,13 @@ private boolean setOnlineStatus(boolean status) { | |||
| if (!repoCfg.contains(online_repo_url)) { | |||
| repoCfg.add(online_repo_url); | |||
| changed = true; | |||
| logger.debug("Added repo '{}' to feature repo list.", online_repo_url); | |||
There was a problem hiding this comment.
Not new, but
online_repo_url => does not follow naming conventions.
| if (installFeature(featuresService, name)) { | ||
| configChanged = true; | ||
| String fullName = PREFIX + PREFIX_PACKAGE + ((String) packageName).trim(); | ||
| if (currentPackage.equals(MINIMAL_PACKAGE)) { |
There was a problem hiding this comment.
Position literals first in String comparisons - that way if the String is null you won't get a NullPointerException, it'll just return false. source: PMD
There was a problem hiding this comment.
We are inside an "instanceof String" check, so we can be sure it is not null.
| } else { | ||
| if (installFeature(featuresService, fullName)) { | ||
| configChanged = true; | ||
| } |
There was a problem hiding this comment.
Should we log anything here or is that already done inside installFeature in the case of failure.
There was a problem hiding this comment.
that's done in installFeature.
Mainly this one: https://github.com/openhab/openhab-core/pull/256/files#diff-3eaf093abf3fdf6aa38a4b358b4c3ca2R488 |
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/why-is-openhab-server-calling-openhab-jfrog-io/45372/50 |
Signed-off-by: Ben Clark <ben@benjyc.uk>
- wait for config update when online status has changed - removed static methods and logger - proper handling of "minimal" feature - introduced a sync job that checks if everything is installed as expected (which serves as a retry mechanism in case of failed downloads) fixes openhab/openhab-distro#602 Signed-off-by: Kai Kreuzer <kai@openhab.org> GitOrigin-RevId: 50c01f2
fixes openhab/openhab-distro#602
Signed-off-by: Kai Kreuzer kai@openhab.org