search

openhab / openhab-distro

The binary distribution of openHAB
https://www.openhab.org/
Eclipse Public License 2.0
1.3k stars 393 forks source link

upgrade.sh issues #816

Open tmrobert8 opened 5 years ago

tmrobert8 commented 5 years ago

While making sure the windows update.ps1 matched the features in update.sh - I believe I saw a number of issues in it (wish I made a list because these are only from memory). Please note that these are based on auditing the code - not actually running the code - so they may not actually be real issues if I misunderstood the code:

  1. scanVersioningList - I'm pretty sure this has the potentially to erroneously apply changes. Let's say you had three versions - 2.4.0, 2.5.0 and 2.6.0 in the update.lst file. If I was on 2.4.0 and wanted to upgrade to 2.5.0 (not 2.6.0) - wouldn't all the 2.5.0 AND 2.6.0 changes be applied? Specifically - it looks like the following line needs an upper bound on it: if [ "$CurrentVersionNumber" -lt "$LineVersionNumber" ]; then

  2. backup/restore on error - the script will backup runtime and userdata/etc and restore those files if the update fails. However, if the unzip of the distribution fails midway (say a space issue) - then you could potentially have changes to the conf and home directories that would not be restored. Seems like the conf and home directory should be backed up as well

  3. scanVersioningList (part 2) - seems like error handling needs to be addressed. If the PRE fails (couldn't move a file or delete a file, etc), the installation continues? Likewise, if the POST fails - shouldn't you backout the changes again (ie do the restore above)? Since the method is making, probably important changes, it should be part of the whole transaction

  4. environment variables - really unsure about this one but it looks like the main unzip of the distribution fully ignores environment variables? In other words, if I have OPENHAB_CONF defined in a different location - the unzip will be unzipping the conf files under OPENHAB_HOME rather than OPENHAB_CONF? Same applies if I've defined OPENHAB_RUNTIME or OPENHAB_USERDATA?

  5. version number - correct me if I'm wrong but it looks like you can downgrade without any notifications. In other words, if I am on version 2.3.0 and do an 'upgrade. 2.1.0' - the script will merrily try to install 2.1.0. On the windows side, I confirm that they really want to do that before continuing (with a strong warning message that the result would be unpredictable).

  6. version number (part 2) - not an issue - but if you try to update to the same version - the script will simply end with "You are already on openHAB x.x.x". I changed that in the windows version to a confirmation whether to continue - that allows the user to REINSTALL the version if they want (a nicety if something's become corrupt). May want to do that in the update.sh as well.

  7. non-issues - in the windows version, I also added a whole bunch of other enhancements that you may want to do in the upgrade.sh version: a version of "2.4" would be translated to "2.4.0", added OPENHAB_RUNTIME as a variable to runCommand(), added error handling (and format validation) to the runCommand version, etc

BClark09 commented 5 years ago

Hi @tmrobert8, sorry for the delay in response.

  1. The update.lst file is pulled from the zip file you're updating to. [2.6.0] would not exist in 2.5.0's .lst file.

  2. Agreed, error handling should be a little more thorough.

  3. Same as 2.

  4. Not sure if I understand, can you point towards any lines you think that do this? The basic order of actions is supposed to be:

    • If there's no $OPENHAB_CONF or $OPENHAB_USERDATA, use the $OPENHAB_HOME/{..}
    • Unzip the contents of the new distro to a temporary folder.
    • Copy over the runtime contents only (we don't want to change dynamic files if we don't have to).
    • Use update.lst to handle $OPENHAB_CONF or $OPENHAB_USERDATA contents specifically.

  5. Correct, a warning should be added to update.sh

  6. Good idea, the same version should be installed over the top. Careful consideration will need to be made about how the scripts handle the update.lst commands though.

  7. x.x to x.x.0 might be a good assumption to make. $OPENHAB_RUNTIME isn't included because currently it can't be anything else other than $OPENHAB_HOME/runtime and didn't want to suggest that this is possible to change.

tmrobert8 commented 5 years ago

@BClark09

  1. I must be more defensive of a programmer. I agree that under normal circumstances it would not - but you are assuming that someone doesn't get a commit through accidentally that has 2.6.0 or that the build process doesn't screw up and include it or any other circumstances. To me, you know your lower bound and upper bound - why not be safe and ensure both are met to cover situations that are unexpected (especially considering that the outcome is file changes if you're assumption is wrong.

  2. Ah - I didn't know update.lst is used for conf/userdata changes. I though the distrubution may have changes as well (like adding new files to the conf)

7 (and a bit of 4 really) - and again being the defensive type ;) I agree that oh2_dir_layout will set it up that way - but why can't the user change that setting to something locally? More importantly - why even have a OPENHAB_RUNTIME variable if it MUST be OPENHAB_HOME/runtime. To me - if you have an environment variable, you are implicitly saying that the user can change that to where ever they want based on their environment. Doesn't make sense to me to then make assumptions in the rest of the code that it will always be the same as OPENHAB_HOME/runtime. If you are forcing it to have a certain structure - get rid of the variable and force the structure. If not, the comply with whereever the variable says it should be and don't make assumptions like that. Just my 2 cents worth...

BClark09 commented 5 years ago

1.) I'm not sure if someone creating PR including an incorrect version, followed by a maintainer reviewing and accepting the change is something that reasonably happen for a release, but thinking about it more there might be chances for [2.5.1] for a patch release and [2.6.0] for snapshots could be mixed together, so yes we definitely should guard against it!

4.) This is how it works at the moment, but i'd be more than happy to have suggestions on better ways of doing this!

7.) I am probably wrong to say this couldn't be changed, and I completely agree that we should only be using variables if they can be... well, varied ;). We'd need to test thoroughly to see it if it has an effect as I haven't seen a use-case for moving it yet.

tmrobert8 commented 5 years ago
  1. When I did the windows version - I unzipped the distribution into a temp directory and then moved any "/conf" to OPENHAB_CONF, any "/userdata" to OPENHAB_USERDATA, any "/runtime" to OPENHAB_RUNTIME, any "/addons" to OPENHAB_ADDONS and what is left (which should be the home directory) to OPENHAB_HOME. Note: like update.sh - we blow away the OPENHAB_RUNTIME and OPENHAB_USERDATA (non etc) first - then do the moves for any new files (ie not overwriting any existing files). That way if there is something new in the conf/userdata - it goes to the proper directories..