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

Add version delimited update list #662

Merged
merged 1 commit into from Apr 17, 2018

Conversation

BClark09
Copy link
Member

Addresses the Linux and Mac OS part of #501

image

Signed-off-by: Ben Clark ben@benjyc.uk

@BClark09
Copy link
Member Author

From 2.1.0:

image

@kaikreuzer
Copy link
Member

Wow, that looks great! A huge step forward 👍

A few small questions:

  • Shouldn't the infos+warnings be shown consolidated, even if multiple version-hops are involved? I mean having lines like "Deleting..." in between can potentially distribute those messages so that they could be easily overlooked/lost.
  • Might it make sense to actually show the infos+warnings even before the user confirms the "Is this okay?"? Maybe we have breaking stuff in an update, which would make the user want to rather postpone the update to a moment when he has the time to do the necessary adaptions. Once upgraded, it might be too late (if he doesn't have a backup to which he can revert to).
  • The infos+warnings are the same content as in the release notes - I would think that they still also make sense in the release notes, so I wonder how we can get to a "single source" approach, so that there is only one place to maintain. Would it be easily possible to extract that content from the update.trans and fill it into a generated markdown document for a specific version?
  • *.trans feels to be a rather unusual file extension. As it is supposed to be used on Windows as well, could you come up with a 3 letter extension?

@BClark09
Copy link
Member Author

Shouldn't the infos+warnings be shown consolidated...

Might it make sense to actually show the infos+warnings even before the user confirms..

Absolutely, This can be set up at the start so we'd have three categories, [[MSG]] [[PRE]] and [[POST]]:

[[MSG]]
[2.1.0]
NOTE;Note 1

[2.2.0]
ALERT;Warning 1

[[PRE]]
...

The infos+warnings are the same content as in the release notes ...

The same script we used to create the changelogs can be extended to use this right? I've got to figure a way of reading this content into linuxpkg.

*.trans feels to be a rather unusual file extension. As it is supposed to be used on Windows as well, could you come up with a 3 letter extension?

.tfr? For transfer?

@BClark09
Copy link
Member Author

image

@kaikreuzer
Copy link
Member

Absolutely, This can be set up at the start so we'd have three categories, [[MSG]] [[PRE]] and [[POST]]

So would you allow NOTES/ALERTS also in the PRE and POST categories? If so, what kind of messages should go where?

The same script we used to create the changelogs can be extended to use this right?

Yes, but for this we should make sure that we have all content in this file that is required for correctly structuring the changelog (we there e.g. have the addon-id and potentially markdown text) - hence I am not sure how to best fit this in here.

.tfr? For transfer?

Maybe update.inf?

@BClark09
Copy link
Member Author

So would you allow NOTES/ALERTS also in the PRE and POST categories? If so, what kind of messages should go where?

Commands can be processed in any section, but at the moment I only expect them in the [[MSG]] category. I'd guess it would be up to a PR reviewer to see if new additions make sense? At the moment, I'm thinking that NOTES should be used where the user might experience unexpected changes in functionality, whereas an ALERT should be used if the user is required to do something.

Yes, but for this we should make sure that we have all content in this file that is required for correctly structuring the changelog (we there e.g. have the addon-id and potentially markdown text) - hence I am not sure how to best fit this in here.

NOTES and ALERTS can be set to take only two parameters, so if we define the extra as unused parameters for the update script, they can be still used by the changelog script instead. For example:

ALERT;Oceanic Binding;The 'softener' Thing Type no longer exists and is replaced by the 'serial' and 'ethernet' Thing Types;<PR Link>;<Other Info>

The finer details can be thought of after this PR?

.inf

.inf might be problematic in Windows as it is very similar but incompatible with the usual .ini/.inf format.

@kaikreuzer
Copy link
Member

NOTES should be used where the user might experience unexpected changes in functionality, whereas an ALERT should be used if the user is required to do something.

Sounds like a good definition.

The finer details can be thought of after this PR?

Agreed, e.g. when addressing #668 :-)

.inf might be problematic in Windows

Not sure if we need to care too much about that as we do not expect any application associations with this extension. But I am fine with other options as well. Just tfr looks for me at a glance like rtfm, which does not bring any positive vibes 😎 .

@BClark09
Copy link
Member Author

BClark09 commented Mar 29, 2018

How about .lst which is commonly used for various plain-text lists? We could use .lst for both this and the systemfile list (if we choose to go down that route). Otherwise, I'm fine with and will change to .inf.

Last thing to consider: At the moment, the update script won't action new commands for snapshots. In other words, updating from a 2.3.0 snapshot to a 2.3.0 snapshot won't action any new commands in the [2.3.0] sections

I wonder what the best strategy for this would be? Is it better in this case repeat all the latest commands on each snapshot-snapshot update?

@kaikreuzer
Copy link
Member

How about .lst

Go for it 👍

I wonder what the best strategy for this would be? Is it better in this case repeat all the latest commands on each snapshot-snapshot update?

I would think so - we should make sure that all those actions are safely repeatable without breaking anything, then they could be executed for every snapshot upgrade (and btw, also for beta and milestone releases).

@BClark09
Copy link
Member Author

BClark09 commented Apr 1, 2018

(and btw, also for beta and milestone releases).

How will these versions be specified in version.properties? If it's "2.3.0-B1" and "2.3.0-M1" then I can look for the "-" and execute the same actions.

@kaikreuzer
Copy link
Member

If it's "2.3.0-B1" and "2.3.0-M1"

Yes, that would be the plan!

@BClark09
Copy link
Member Author

BClark09 commented Apr 2, 2018

All set, upgrades work to 2.3.0-SNAPSHOT from 2.1.0, 2.2.0 and 2.3.0 on Linux. @kaikreuzer, would you be able to test on MacOS?

If it helps, my usual test to see if the upgrade completes all steps are:

  1. Use the build as a snapshot file I've built the distro from this PR here:
  2. Download an existing openHAB installation, and extract it to a new location.
  3. In the openHAB installation replace the following line (101) in runtime/bin/update
    curl -Lf# "$DownloadLocation" -o "$OutputFile" || {
    
    with
    cp [zip location from step 1] "$OutputFile" || {
    
  4. Run ./runtime/update 2.3.0-SNAPSHOT

@BClark09
Copy link
Member Author

BClark09 commented Apr 2, 2018

@openhab/docker-maintainers, @openhab/synology-maintainers, @openhab/qnap-maintainers, @bdleedy, @ghys (windows script). Using the change in this PR, would it be possible to complete the update procedure for the relevant system, or is something else required?

In summary, the update procedure used by this PR is:

  1. Use the version of runtime/bin/update.lst inside the compressed folder (place in temporary folder).
  2. Display the relevant notes alerts under the [[MSG]] category.
  3. If the update procedure is interactive, then ask the user if they want to continue:
  4. Perform the tasks under the [[PRE]] category, expanding the $OPENHAB_* variables to the correct path.
  5. Remove all files inside runtime, either by deleting them or moving them to a temporary location.
  6. Remove the system files inside userdata/etc there will probably be a file containing this list later on.
  7. Extract the contents of the new version zip, making sure not to replace any file that already exists.
  8. Perform the tasks under the [[POST]] category, expanding the $OPENHAB_* variables to the correct path.
  9. Delete userdata/tmp and userdata/cache.

@kaikreuzer
Copy link
Member

@kaikreuzer, would you be able to test on MacOS?

Tested, works well!

Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

Lgtm, is there anything more you would want to add or wait for or shall we merge?

@cniweb
Copy link
Member

cniweb commented Apr 3, 2018

If the update procedure is interactive, then ask the user if they want to continue

How can we run this update procedure non interactive?
When updating the Docker image or the Synology package, there are no interactive input options!

@BClark09
Copy link
Member Author

BClark09 commented Apr 3, 2018

If the update can't have input options, then it would probably be best to display the lines on screen or write the messages to an easy to read file?

Lgtm, is there anything more you would want to add or wait for or shall we merge?

Not from me, I'll have a look to see how I can integrate this with Linuxpkg.

@cniweb
Copy link
Member

cniweb commented Apr 4, 2018

Is there an option for the update to go through without interactive input?

@BClark09
Copy link
Member Author

BClark09 commented Apr 4, 2018

If you are able to directly use the update script, then I can add a non-interactive option into it?

@BClark09
Copy link
Member Author

BClark09 commented Apr 5, 2018

@kaikreuzer, I am wondering if we should change the use of

MOVE;[*];[*].bak

to

DEFAULT;[*]

This way, we can differentiate between needing to move a file, and needing to replace one with a new version. This is probably necessary for Linuxpkg, which detects new versions of the file automatically.

@kaikreuzer
Copy link
Member

@BClark09 Not sure if I fully understand the suggestion. What exactly will DEFAULT do? I agree that we should not use MOVE in order to create backups of files that we want to replace.

@BClark09
Copy link
Member Author

BClark09 commented Apr 9, 2018

In linuxpkg, apt and rpm packages automatically detect when the maintainer changes a file that is normally a configuration file. DEFAULT was just a suggestion to tell the upgrade process: "This file needs to be replaced with the distribution default." In reality for manual installations, this is just a rename of the file to *.bak.

@kaikreuzer
Copy link
Member

Meaning, you can do a specific processing of that file for the linuxpkg upgrades? Sure, let's go for that.

@cniweb
Copy link
Member

cniweb commented Apr 9, 2018

In linuxpkg, apt and rpm packages automatically detect when the maintainer changes a file

How can I use this process for docker and synology, too?

@BClark09
Copy link
Member Author

Hi @cniweb, sorry for the delay!

To bring discussions into a single thread:

It would be nice if there is a backup / update script that would work for all packages (linuxpkg, windows, docker, synology, rpm, apt, etc.)

Without bringing in dependencies, Windows will have to have separate scripts (AFAIK). However, I have tried to make the backup, restore and update shell scripts (in the runtime) POSIX compliant., but they'll need zip and unzip and curl respectively to work. Unfortunately I am very unfamiliar with Docker and Synology so I don't know if the following will work:

You should be able to use the backup and restore scripts if each of the $OPENHAB_* variables are set (paths pointing to the openHAB runtime, conf, userdata folder etc.) and if the running script has access to the /tmp/ directory.

The update script will need to be modified to use the environment variables like the backup and restore scripts. It will also need a non-interactive mode if it is necessary for Docker and Synology.

What do you think?

@cniweb
Copy link
Member

cniweb commented Apr 11, 2018

I think that's a good way.

Both platforms can handle the appropriate environment variables.

The dependent packages (like zip, curl and so on) already exist!

@BClark09 BClark09 force-pushed the update-delim-list branch 3 times, most recently from b3d676d to ca06969 Compare April 17, 2018 12:16
 - Added non-interactive mode

Signed-off-by: Ben Clark <ben@benjyc.uk>
@BClark09
Copy link
Member Author

@cniweb, I've added a check for a $OPENHAB_NONINTERACT environment variable, if it's set to anything (e.g. "true") then the update continues without prompts.

@kaikreuzer, would you be able to do one more review/test? I made a git error and had to rebase, I believe all the changes are still intact though.

Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

lgtm - didn't do another test, but I am sure we will notice it soon, if it is broken ;-)

@kaikreuzer kaikreuzer merged commit 3d2fc78 into openhab:master Apr 17, 2018
@BClark09 BClark09 deleted the update-delim-list branch May 2, 2018 14:51
@wborn wborn added this to the 2.3 milestone Feb 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants