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.

Sign up for GitHub

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

Jump to bottom

Improved the feature installation logic #256

Merged
merged 1 commit into from Dec 12, 2017
Merged

Improved the feature installation logic #256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion bundles/org.openhab.core.karaf/META-INF/MANIFEST.MF
View file
Open in desktop
Expand Up @@ -3,10 +3,11 @@ Bundle-ActivationPolicy: lazy
Bundle-ManifestVersion: 2
Bundle-Name: openHAB Karaf Integration
Bundle-RequiredExecutionEnvironment: JavaSE-1.8
Bundle-SymbolicName: org.openhab.core.karaf
Bundle-SymbolicName: org.openhab.core.karaf;singleton:=true
Bundle-Vendor: openHAB
Bundle-Version: 2.2.0.qualifier
Import-Package:
com.google.common.base,
com.google.common.collect,
org.apache.commons.lang,
org.apache.karaf.features,
Expand Down
62 changes: 48 additions & 14 deletions ...rg.openhab.core.karaf/src/main/java/org/openhab/core/karaf/internal/FeatureInstaller.java
View file
Open in desktop
Expand Up @@ -19,12 +19,15 @@
import java.util.EnumSet;
import java.util.HashSet;
import java.util.Hashtable;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Properties;
import java.util.Set;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;

import org.apache.commons.lang.StringUtils;
Expand All @@ -42,6 +45,9 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import com.google.common.collect.Iterators;
import com.google.common.collect.Maps;

/**
* This service reads addons.cfg and installs listed addons (= Karaf features) and the selected package.
* It furthermore allows configuration of the base package through the Paper UI as well as administrating Karaf to
Expand All @@ -51,10 +57,12 @@
*/
public class FeatureInstaller implements ConfigurationListener {

public static final String MINIMAL_PACKAGE = "minimal";
private static final String CFG_REMOTE = "remote";
private static final String CFG_LEGACY = "legacy";

private static final String PAX_URL_PID = "org.ops4j.pax.url.mvn";
private static final String ADDONS_PID = "org.openhab.addons";
private static final String PROPERTY_MVN_REPOS = "org.ops4j.pax.url.mvn.repositories";

public static final String STANDARD_PACKAGE = "standard";
Expand All @@ -64,7 +72,7 @@ public class FeatureInstaller implements ConfigurationListener {
public static final String[] addonTypes = new String[] { "binding", "ui", "persistence", "action", "voice",
"transformation", "misc" };

private static final Logger logger = LoggerFactory.getLogger(FeatureInstaller.class);
private final Logger logger = LoggerFactory.getLogger(FeatureInstaller.class);

private String online_repo_url = null;
private URI legacy_addons_url = null;
Expand All @@ -73,6 +81,8 @@ public class FeatureInstaller implements ConfigurationListener {
private ConfigurationAdmin configurationAdmin;
private static EventPublisher eventPublisher;

private ScheduledExecutorService scheduler;

private boolean paxCfgUpdated = true; // a flag used to check whether CM has already successfully updated the pax
// configuration as this must be waited for before trying to add feature repos

Expand Down Expand Up @@ -106,14 +116,34 @@ protected void activate(final Map<String, Object> config) {
setOnlineRepoUrl();
setLegacyRepoUrl();
modified(config);
scheduler = Executors.newSingleThreadScheduledExecutor();
Copy link
Member

Choose a reason for hiding this comment

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

I would maybe initialize where I declare it, for me that feels safer.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, because it needs to be (re-)initialized upon activation and shut down on deactivation.

scheduler.scheduleWithFixedDelay(() -> {
logger.debug("Running scheduled sync job");
try {
Dictionary<String, Object> cfg = configurationAdmin.getConfiguration(ADDONS_PID).getProperties();
Iterator<String> keysIter = Iterators.forEnumeration(cfg.keys());
Copy link
Member

Choose a reason for hiding this comment

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

:-( unfortunately this is the best way which I can also find..

Map<String, Object> cfgMap = Maps.toMap(keysIter, cfg::get);
modified(cfgMap);
} catch (IOException e) {
logger.debug("Failed to retrieve the addons configuration from configuration admin: {}",
e.getMessage());
}
}, 1, 1, TimeUnit.MINUTES);
}

protected void deactivate() {
if (scheduler != null) {
scheduler.shutdownNow();
scheduler = null;
}
}

protected void modified(final Map<String, Object> config) {
boolean changed = false;
boolean online = (config.get(CFG_REMOTE) == null && getOnlineStatus())
|| (config.get(CFG_REMOTE) != null && "true".equals(config.get(CFG_REMOTE).toString()));
if (getOnlineStatus() != online) {
setOnlineStatus(online);
changed = setOnlineStatus(online);
}

final boolean configChanged = changed;
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

Not new, but

online_repo_url => does not follow naming conventions.

Copy link
Member Author

Choose a reason for hiding this comment

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

right, not new.

}
} else {
if (repoCfg.contains(online_repo_url)) {
repoCfg.remove(online_repo_url);
changed = true;
logger.debug("Removed repo '{}' from feature repo list.", online_repo_url);
}
}
if (changed) {
Expand Down Expand Up @@ -413,7 +445,7 @@ private synchronized void uninstallFeatures(FeaturesService featuresService, Set
}
}

private static synchronized boolean installFeature(FeaturesService featuresService, String name) {
private synchronized boolean installFeature(FeaturesService featuresService, String name) {
try {
Feature[] features = featuresService.listInstalledFeatures();
if (!isInstalled(features, name)) {
Expand All @@ -431,7 +463,7 @@ private static synchronized boolean installFeature(FeaturesService featuresServi
return false;
}

private static synchronized boolean uninstallFeature(FeaturesService featuresService, String name) {
private synchronized boolean uninstallFeature(FeaturesService featuresService, String name) {
try {
Feature[] features = featuresService.listInstalledFeatures();
if (isInstalled(features, name)) {
Expand All @@ -446,24 +478,26 @@ private static synchronized boolean uninstallFeature(FeaturesService featuresSer
return false;
}

private static synchronized boolean installPackage(FeaturesService featuresService,
final Map<String, Object> config) {
private synchronized boolean installPackage(FeaturesService featuresService, final Map<String, Object> config) {
boolean configChanged = false;
Object packageName = config.get(OpenHAB.CFG_PACKAGE);
if (packageName instanceof String) {
currentPackage = (String) packageName;
String name = PREFIX + PREFIX_PACKAGE + ((String) packageName).trim();
if (installFeature(featuresService, name)) {
configChanged = true;
String fullName = PREFIX + PREFIX_PACKAGE + ((String) packageName).trim();
if (currentPackage.equals(MINIMAL_PACKAGE)) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

We are inside an "instanceof String" check, so we can be sure it is not null.

Copy link
Member

Choose a reason for hiding this comment

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

But is still a good habit 😜

// no changes are done to the add-ons list, so the installer should proceed
configChanged = false;
} else {
if (installFeature(featuresService, fullName)) {
configChanged = true;
}
Copy link
Member

Choose a reason for hiding this comment

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

Should we log anything here or is that already done inside installFeature in the case of failure.

Copy link
Member Author

Choose a reason for hiding this comment

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

that's done in installFeature.

}

// uninstall all other packages
try {
for (Feature feature : featuresService.listFeatures()) {
if (feature.getName().startsWith(PREFIX + PREFIX_PACKAGE) && !feature.getName().equals(name)) {
if (uninstallFeature(featuresService, feature.getName()) && !configChanged) {
configChanged = true;
}
if (feature.getName().startsWith(PREFIX + PREFIX_PACKAGE) && !feature.getName().equals(fullName)) {
uninstallFeature(featuresService, feature.getName());
}
}
} catch (Exception e) {
Expand All @@ -473,7 +507,7 @@ private static synchronized boolean installPackage(FeaturesService featuresServi
return configChanged;
}

private static boolean isInstalled(Feature[] features, String name) {
private boolean isInstalled(Feature[] features, String name) {
try {
for (Feature feature : features) {
if (feature.getName().equals(name)) {
Expand Down