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

Improved the feature installation logic #256

Merged
merged 1 commit into from Dec 12, 2017

Conversation

Projects
None yet
3 participants
@kaikreuzer
Copy link
Member

commented Dec 11, 2017

  • 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

Improved the feature installation logic
- 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>
@martinvw
Copy link
Member

left a 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();

This comment has been minimized.

Copy link
@martinvw

martinvw Dec 12, 2017

Member

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

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Dec 12, 2017

Author Member

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());

This comment has been minimized.

Copy link
@martinvw

martinvw Dec 12, 2017

Member

:-( 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);

This comment has been minimized.

Copy link
@martinvw

martinvw Dec 12, 2017

Member

Not new, but

online_repo_url => does not follow naming conventions.

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Dec 12, 2017

Author Member

right, not new.

if (installFeature(featuresService, name)) {
configChanged = true;
String fullName = PREFIX + PREFIX_PACKAGE + ((String) packageName).trim();
if (currentPackage.equals(MINIMAL_PACKAGE)) {

This comment has been minimized.

Copy link
@martinvw

martinvw Dec 12, 2017

Member

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

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Dec 12, 2017

Author Member

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

This comment has been minimized.

Copy link
@martinvw

martinvw Dec 12, 2017

Member

But is still a good habit 😜

} else {
if (installFeature(featuresService, fullName)) {
configChanged = true;
}

This comment has been minimized.

Copy link
@martinvw

martinvw Dec 12, 2017

Member

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

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Dec 12, 2017

Author Member

that's done in installFeature.

@kaikreuzer

This comment has been minimized.

Copy link
Member Author

commented Dec 12, 2017

@martinvw martinvw merged commit 50c01f2 into openhab:master Dec 12, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kaikreuzer kaikreuzer deleted the kaikreuzer:featint branch Dec 12, 2017

@kaikreuzer kaikreuzer modified the milestone: 2.2 Dec 15, 2017

@openhab-bot

This comment has been minimized.

Copy link
Collaborator

commented Nov 6, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.