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

[updateopenhab] [experimental] initial contribution #10622

Closed
wants to merge 19 commits into from

Conversation

andrewfg
Copy link
Contributor

This is a first attempt at creating a binding which supports a thing that implements automatic updating of OpenHAB.

This PR is made available to allow OpenHAB maintainers to review the code and give feedback.

It is only a proof of concept (or not). It is not intended to be merged at this time.

Signed-off-by: Andrew Fiddian-Green software@whitebear.ch

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@andrewfg andrewfg added discussion new binding If someone has started to work on a binding. For a new binding PR. awaiting feedback additional testing preferred The change works for the pull request author. A test from someone else is preferred though. work in progress A PR that is not yet ready to be merged labels Apr 30, 2021
@andrewfg andrewfg requested a review from a team as a code owner April 30, 2021 18:56
@fwolter
Copy link
Member

fwolter commented May 1, 2021

I think this is a quite convenient way to update OH, especially for new users. But I don't think a binding is the right approach for implementing this. Did you consider adding this to the core and the UI?

@andrewfg
Copy link
Contributor Author

andrewfg commented May 1, 2021

Did you consider adding this to the core and the UI?

I did. But my current skill set is greater in Binding development than in Core or UI development. And the deployment on different machines for testing purposes is easier as well. So I would like to continue developing the code within the Binding model, and when it is fully ready and tested as a Binding, we could migrate the code base into Core resp. UI modules. => Is that Ok with you @fwolter ?

@fwolter
Copy link
Member

fwolter commented May 1, 2021

Sure, but also demand some feedback from core maintainers.

@hmerk
Copy link
Contributor

hmerk commented May 1, 2021

I also would see this as a core component instead of a binding, so there would be no need to install this additionally and would like to have it as a fixed entry in the UI...

Anyway, good starting point Andrew👍

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@andrewfg
Copy link
Contributor Author

andrewfg commented May 2, 2021

I just pushed some more commits, and it is now almost working!

What works:

  • UI displays actual running version and version available for download
  • Linux and Windows: script files can be successfully written to disk, and execution started
  • Linux: system stops when the script starts and restarts again when it is finished
  • Windows: system stops when the script starts, but ... see below

What does not work (yet):

  • The script commands to do the actual update, are still commented out (makes it easier to test initially)
  • Linux: 'KARAF_HOME' and 'mkdir' error messages
  • Windows: system fails to restart when script is finished

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/addon-to-update-oh-thoughts-anyone/121576/8

@digitaldan
Copy link
Contributor

Nice start!
Question, for apt-get on debian based systems, how is the Java process, which usually runs under the "openhab" user able to install/update deb packages, which usually requires root user permissions?

updater = new MacUpdater(targetVersion, config.password);
break;
case UNIX:
updater = new DebianUpdater(targetVersion, config.password);
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if this is a non Debian based system like Redhat or Gentoo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Each version has a class of Updater that extends BaseUpdater. I wrote three so far — namely WindowsUpdater, DebianUpdater, and MacUpdater. The former two, I can test because I have such computers. The latter is basically a guess; half way between Windows and Debian, I think. Any other variety of system would need volunteers to write and test (hopefully largely copy/ paste/modify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PS I have also written an interface decoupler class that reads/writes between the xxUpdater class and the OH Thing handler, so one can change the binding, or the updater, fully independently.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, you may want to check for a debian flavor specifically, something like

Files.readString(Path.of("/preoc/version")).indexOf("Debian") > 0

so not to match non debian based OSs

Copy link
Contributor Author

@andrewfg andrewfg May 9, 2021

Choose a reason for hiding this comment

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

Hi @digitaldan many thanks for the suggestion. Just for info, it is not so much the OS distro that needs to be detected, but rather their respective Package Manager (which we use to run the actual update) -- be it debian (apt), rpm (yum, dnf), gentoo etc. Anyway I think the better place to discover more appropriate details is in /etc/os-release - any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

be it debian (apt), rpm (yum, dnf), gentoo etc.

I was just using Debian as an example here as thats what you have named the the updater class, but checking for package manager vs distro sounds like a good plan.

yway I think the better place to discover more appropriate details is in /etc/os-release - any thoughts?

Sounds good to me, my suggestion for the version was just what i know off the top of my head :-)

@digitaldan
Copy link
Contributor

Nm, i see you pass a password into sudo from the binding config, nice!

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@andrewfg
Copy link
Contributor Author

I created this PR openhab/openhab-core#2372 to add the code to the OH core (rather than a binding), so I will now close this binding PR.

@andrewfg andrewfg closed this May 19, 2021
@wborn wborn removed the additional testing preferred The change works for the pull request author. A test from someone else is preferred though. label Dec 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting feedback discussion new binding If someone has started to work on a binding. For a new binding PR. work in progress A PR that is not yet ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants