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

Fix Windows update.ps1 backup command and add version auto increment feature #622

Closed
wants to merge 4 commits into from

Conversation

bdleedy
Copy link
Contributor

@bdleedy bdleedy commented Dec 18, 2017

  • Changed backup command to use $OHDirectory to fix issue where thedefined update parameter would not be passed.
  • Removed default version of 2.1.0 and added auto increment feature

Signed-off-by: Brian Leedy awsnap@gmail.com

defined update parameter would not be passed.
- Removed default version of 2.1.0 and added auto increment feature

Signed-off-by: Brian Leedy awsnap@gmail.com
@bdleedy
Copy link
Contributor Author

bdleedy commented Dec 18, 2017

@tmrobert8 @BClark09 I was feeling inspired (also healthier than last week) so I fixed the directory parameter that update sends backup, removed the default version of 2.1.0, and built an auto increment like the linux version has. If you're not tired of testing my stuff, have a go.

@tmrobert8
Copy link
Contributor

So - I execute all the PS1's from a .bat file with the following line:

powershell -command "& { . .\update.ps1; Update-openHAB -OHDirectory c:\openhab2.2 -OHVersion 2.2.0 -Snapshot $true }"

When I ran that with your updated script - I got the following:

Backup created at c:\openhab2.2\backups\openhab2-backup-201712181546.zip
Backup script finished.

Downloading the openHAB 2.2.0-SNAPSHOT distribution...
Exception calling "GetResponse" with "0" argument(s): "The remote server returned an error: (404) Not Found."
At C:\openhab2.2\runtime\bin\update.ps1:15 char:5
+     $response = $request.GetResponse()
+     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (:) [], MethodInvocationException
    + FullyQualifiedErrorId : WebException

which is fine - however, the script continued to run, causing HUGE amounts of error messages (nulls, missing directories, etc).

You may want to check the response from the web and stop processing after that.

@bdleedy
Copy link
Contributor Author

bdleedy commented Dec 18, 2017

If you look, you'll see that I do. Apparently not correctly though. It threw for me when it couldn't find the non snapshot. I'll take a look.

EDIT: I see you're talking about the actual download. I was still in autoincrement mode. Maybe I do a check like I do there?

BTW I do agree with your assessment that the update script running twice if it's the same file is silly but I left that to a later date to get this done for this version. It's the way the Linux one does it so I just went with that. I had some ideas like you.

Compare a version line in each file (follow the OH version but use the 4th 'private place, 2.2.0.1, 2.2.0.2, etc)
Compare hash of each file
Compare size of each file

@tmrobert8
Copy link
Contributor

Another nice to have - yell at them if the current directory is one of those directories you will be deleting (like cache or tmp). Mainly their fault for being in those directories - but you can probably stave off some bug reports by warning them to move.

Can't get the download to work - either snapshot or non-snapshot - I always get 404's. Are the directories correct (I must admit - I don't have the time right now to check)..

@bdleedy
Copy link
Contributor Author

bdleedy commented Dec 18, 2017

OK, got ya. How about if the current directory is not the specified one but contains the specified one?

Download works just fine for me. It reports that it's downloading the openHAB 2.2.0 distribution? Seems like we should still throw an error though.

…'s are not the same the

newer one is used.
- Added checks for files existance before downloading
- Added check if current directory is inside of $OHDirectory
- Moved setting of download locations earlier
- Simplified download since check for $Snapshot is handled in download
location logic
@bdleedy
Copy link
Contributor Author

bdleedy commented Dec 19, 2017

  • Added check for ScriptVersion in new script check, if ScriptVersion's are not the same the newer one is used. (this is throw an error currently because there is no scriptversion in the current update but it seems silly to write code to go around this temporarily)
  • Added checks for files existence before downloading
  • Added check if current directory is inside of $OHDirectory
  • Moved setting of download locations earlier
  • Simplified download since check for $Snapshot is handled in download location logic

@tmrobert8 I checked again and the stable download works for me. I didn't test snapshot.

@tmrobert8
Copy link
Contributor

tmrobert8 commented Dec 19, 2017

@bdleedy
Check for current directory didn't work. Looking at the code - you have the condition reversed - instead of $OHDirectory.Contains($CurrentDir) - should be $CurrentDir.Contains($OHDirectory)

Found out why I was getting 404's on the download and this is something you should address - because an earlier version blew up halfway through the process - I didn't have a version.properties file (because it was deleted from that earlier run). Without it and with no -OHVersion specified - the $OHVersion was blank. You need to add an ELSE to the Test-Path ".../version.properties" saying that no version was found and to specify -OHVersion (or something to that effect).

However - when I then added -OHVersion to the command line - the backup.ps1 was blowing up left and right because there was no "version.properties" file. You probably need to address the same issue in backup.ps1

Finally - your new scriptversion check is great - but you need to make sure you handle the situation where there is NO scriptversion line:

Finished Download
You cannot call a method on a null-valued expression.
At C:\openhab2.2\runtime\bin\update.ps1:53 char:13
+             $NewScriptVersion = $NewScriptVersionLine.Split(":")[1]
+             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : InvalidOperation: (:) [], RuntimeException
    + FullyQualifiedErrorId : InvokeMethodOnNull

If there is no script version - I'd just avoid touching this code and doing the double download (as before).

BTW - this script is really great so far!

@bdleedy
Copy link
Contributor Author

bdleedy commented Dec 19, 2017

@tmrobert8
Directory: Whoops. I threw this in quick, did some testing in PS console and just edited the commands, not variables back in ISE.

Version.Properties: I don't know how often this would actually occur but I'll add the else here and in backup.

ScriptVersion: See my comments. This throws this error because it's not in the current one. It would never happen in production. Seems silly to write something to handle that. Right?

@tmrobert8
Copy link
Contributor

tmrobert8 commented Dec 19, 2017

ScriptVersion - yeah makes sense - I was thinking of when they upgrade to this but didn't really think it through.

Version.properties: I don't know how often it would happen but it's happen to me twice now ;) I'd definitely put in a check because there is alot of processing that could go wrong between deleting it and trying to recreate it. I'm thinking either the user cancelling it in between or running out of disk space copying the new runtime (two most likely cases). I'd actually be tempted to move those removes to right before copying the new userdata stuff - that way you minimize the in-between time/processing (like you do with runtime - delete then immediately create).

Super bonus points but would really be cool - If you wanted a super safe script, go the way of installers and catch any exception after you do the backup and restore the backup if an exception occurs (so they are back in the same spot they were prior to everything). But that may be going waaay beyond the scope here ;)

@bdleedy
Copy link
Contributor Author

bdleedy commented Dec 19, 2017

Which removes do you speak of? The big block of them? I'm not sure I can get it any closer. Any other remove before that is strictly temp files unless I'm missing something. I'll push commits when we figure this out.

Super bonus points IS the goal I've had all along. Maybe we can get that in before the next release too.

@tmrobert8
Copy link
Contributor

tmrobert8 commented Dec 19, 2017

Moving was just a suggestion - but if it were me, I'd move that big block of userdata remove-items (lines 290-305) to just after the runtime copy (line 316). That way you remove them just before copying the new ones over

- Reconfigured remove/copy structure to handle a folder at a time
@bdleedy
Copy link
Contributor Author

bdleedy commented Jan 8, 2018

@tmrobert8 Can you validate this so we can marge it?

@tmrobert8
Copy link
Contributor

@bdleedy
Sorry it took me awhile to get to this:

C:\openhab2.2>powershell -command "& { . .\runtime\bin\update.ps1; Update-openHAB -OHVersion 2.2.0 }"
Checking the specified openHAB directory...
Checking that current directory is not within openHAB root...
You are in a folder that may need deleting/altering. Please change your current directory to the openHAB root or
outside of it.
At C:\openhab2.2\runtime\bin\update.ps1:129 char:13
+             throw "You are in a folder that may need deleting/alterin ...
+             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : OperationStopped: (You are in a fo... outside of it.:String) [], RuntimeException
    + FullyQualifiedErrorId : You are in a folder that may need deleting/altering. Please change your current director
   y to the openHAB root or outside of it.


C:\openhab2.2>

@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/wintoolhab-windows-backup-restore-update-tool-for-openhab/40051/7

@bdleedy
Copy link
Contributor Author

bdleedy commented Mar 7, 2018

I started to type "I cannot replicate this..." and then realized that I was searching looking for "." in "pwd".

I'm going to set the default $OHDirectory to (pwd).ToString().ToLower() instead of "."

-Used pwd instead of . to defined current working directory

Signed-off-by: Brian Leedy awsnap@gmail.com
@kaikreuzer
Copy link
Member

Sorry, I lost this a bit out of sight and by now, we have a conflict.
@bdleedy Where are we with that? Besides the conflict, would you say this is ready to get merged?

@kaikreuzer
Copy link
Member

@bdleedy Short friendly ping!

@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/windows-update-script/33368/6

@kaikreuzer
Copy link
Member

Hey @bdleedy, half a year later, another friendly ping!
Are you still around? I'd hope so! But if not, please let us know, so that maybe @tmrobert8 could take over here?

@tmrobert8
Copy link
Contributor

Let me know if you want me to do anything...

@kaikreuzer
Copy link
Member

As @bdleedy doesn't respond anymore, it would be great if you could take over this PR (i.e. take the content and create a new PR from it).

@tmrobert8
Copy link
Contributor

Just forked the repository. I know #716 and #784 along with some other things I've noticed - but are there any others I should be aware of?

@kaikreuzer
Copy link
Member

I don't really now. A few things have been done on the update script for Linux/Mac, so this might not be really in sync anymore, but we won't have dedicated issues either. @BClark09 might want to chime in here.

@bdleedy
Copy link
Contributor Author

bdleedy commented Nov 12, 2018

Sorry. Life has inevitably gotten in the way. If you ping me with what you come up with, I'll be a second set of eyes but I just don't have the passion or time to contribute right now. Sorry.

@tmrobert8
Copy link
Contributor

Brian - no problems. I've taken what you did (which is great) and ran with it a bit. I'll be committing something this week with the following changes:

  1. Updated the code to the latest version of powershell
  2. Made a common modules script to share common code between them all
  3. Improved the error handling of backup/update scripts
  4. Created a restore script that mirrors (kinda) the linux version and updated the update one to restore if the update fails midway
  5. Fixed a few issues in the update script
  6. Made the backup zip compatible with the linux one (so they are kinda interchangable) - still need to test/verify this one. The biggest issue here is the linux one stores ownership information and tries to restore that - which I think is a BIG mistake (making it very non-portable between systems regardless of OS). Maybe @kaikreuzer can comment on that.

Once I commit - would love a second set of eyes to see what I missed and to obviously test it out..

Tim

@tmrobert8
Copy link
Contributor

tmrobert8 commented Nov 13, 2018

Forgot:
7. Added .bat files to call the powerscript ones to make it easier to execute (for those that are lazy like me!)

@bdleedy
Copy link
Contributor Author

bdleedy commented Dec 12, 2018

Code surpassed by PR #809
Closing this one.

@bdleedy bdleedy closed this Dec 12, 2018
@bdleedy bdleedy deleted the updateps1 branch December 12, 2018 16:03
@kaikreuzer
Copy link
Member

Hooray, zero open PRs now on this repo 🎉

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