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

[addonservices] allow offline mode #2633

Merged
merged 2 commits into from
Jan 1, 2022

Conversation

J-N-K
Copy link
Member

@J-N-K J-N-K commented Dec 20, 2021

Fixes #2628

Signed-off-by: Jan N. Klug jan.n.klug@rub.de

Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>
@J-N-K J-N-K requested a review from a team as a code owner December 20, 2021 17:09
@J-N-K
Copy link
Member Author

J-N-K commented Dec 20, 2021

Since there is a lot of common code between both addon services, it might be a good idea to refactor them to inherit from an AbstractAddonService. But this is out of scope here and should probably be done after a solution for #2607 is found.

@J-N-K
Copy link
Member Author

J-N-K commented Dec 20, 2021

Test fails unrelated.

Copy link
Contributor

@cweitkamp cweitkamp left a comment

Choose a reason for hiding this comment

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

Thank you for looking into this. From my pov we now reached some point where we need unit tests to cover the functionality of those addon services. Wdyt?
If you like I can give you a hand on that. Just ping me.

And I agree. The code can be reduced by introducing an abstract base class.

@cweitkamp cweitkamp added the enhancement An enhancement or new feature of the Core label Dec 29, 2021
@J-N-K
Copy link
Member Author

J-N-K commented Dec 29, 2021

Regarding the unit tests: I agree. Unfortunately it's not that easy to mock the remote end (and it's also different for both add-on services.

This will improve with the introduction of the AbstractAddonService as a TestAddonService could at least test the common parts.

IMO the bundle installers also need test coverage. I can have a look in a separate PR.

Signed-off-by: Jan N. Klug <github@klug.nrw>
@J-N-K J-N-K changed the title [addonservice] allow offline mode [addonservices] allow offline mode Dec 30, 2021
Copy link
Contributor

@cweitkamp cweitkamp 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.

🎆 Happy New Year.

@cweitkamp cweitkamp merged commit d420bee into openhab:main Jan 1, 2022
@cweitkamp cweitkamp added this to the 3.3 milestone Jan 1, 2022
@J-N-K J-N-K deleted the addonservice-offline branch January 1, 2022 11:08
private boolean remoteEnabled() {
try {
Configuration configuration = configurationAdmin.getConfiguration("org.openhab.addons", null);
return (boolean) Objects.requireNonNullElse(configuration.getProperties().get("remote"), true);
Copy link
Contributor

@cweitkamp cweitkamp Jan 6, 2022

Choose a reason for hiding this comment

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

It looks like the getProperties() can return null and we are potentially run into NPEs here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in fc25ddb

Copy link
Contributor

@cweitkamp cweitkamp Jan 8, 2022

Choose a reason for hiding this comment

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

Thank you.

I will try to find a solution for #2581 based on ExpiringCacheMap later.

splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 12, 2023
* allow offline mode

Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>
GitOrigin-RevId: d420bee
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.

[marketplace] Expecting no access to the marketplace when remote is set to false
2 participants