StorminStanley / st2workroom

Vagrant environment used to play with StackStorm, develop StackStorm for your environment, or develop on StackStorm itself!
Apache License 2.0
23 stars 21 forks source link

Upgrade st2 check #326

Closed manasdk closed 8 years ago

manasdk commented 8 years ago

Added a script that checks if st2 could be upgrade and ask user to confirm if st2 version is to be changed. Took this approach after some local conversation and the realization that it is better than mucking around with json or yaml files in bash. It is likely better to ask users to make an explicit choice and there is an added benefit of knowing there is an upstream update.

Note: KB is yet to be written.

Testing -

# ./upgrade_st2_check.sh
We noticed that the currently 1.2.0-8 and 1.3.0-9  are different.
Version 1.3.0-9  will overwrite 1.2.0-8.
To avoid seeing this message in the future please follow instructions from <KB>.
Do you wish to continue with the installation? [Y/n]Y
[root@manas-v1 script]# echo $?
0

If n is selected

# ./upgrade_st2_check.sh
We noticed that the currently 1.2.0-8 and 1.3.0-9  are different.
Version 1.3.0-9  will overwrite 1.2.0-8.
To avoid seeing this message in the future please follow instructions from <KB>.
Do you wish to continue with the installation? [Y/n]n
[root@manas-v1 script]# echo $?
1

p.s: If it is not obvious after reading the script I am a bash n00b. Will appreciate tips on how to write this better ;)

manasdk commented 8 years ago

/cc @Kami @jfryman

Appreciate :eyes:

dzimine commented 8 years ago

Thanks @manasdk

Kami commented 8 years ago

How does this after the graphical installer (notably the y/n prompt)?

I can't remember if the GUI installer also runs bootstrap-st2 or it just does puppet-apply (if it does the latter we should be fine).

Kami commented 8 years ago

Overall the approach and script looks good to me, but we definitely want automated tests for this (especially for all the edge cases / branches).

To make it easier to test we will probably need to change some path variables to take a default value from environment variables and use different exit codes in different if branches, but it should be doable.

Kami commented 8 years ago

Also, it looks like you decided not to go with the "automatically pin user to the current version and proceed with the update system" approach we discussed before.

Any particular reason?

The reason why I think that approach is probably better is because update-system in our cases serves as a general convergence system. This means if all the values provided by the user are pinned / the same the outcomes should be the same as well.

And the same approach (running update-system) is used by users a lot to "fix" different issues by causing convergence to a defined state (because of a previous partial failure or similar).

To clarify - I'm fine with not auto writing the version, but if we don't do that we should write some docs / KB article on how to continue with update system by pinning installation to the current version (put version in hieradata / answers file, etc.).

manasdk commented 8 years ago

GUI installer also runs bootstrap-st2 or it just does puppet-apply

https://github.com/StackStorm/st2installer/blob/master/st2installer/controllers/root.py#L38

I think we are good there since its likely this script will not even run.

manasdk commented 8 years ago

Also, it looks like you decided not to go with the "automatically pin user to the current version and proceed with the update system" approach we discussed before. Any particular reason?

  1. Writing to json or yaml files from bash is rather painful.
  2. We do not modify answers files anywhere so this would be surprising behavior to a user that provides an answers file.
  3. This behavior is explicit and there is nothing implicit so hopefully minimal surprises.

but if we don't do that we should write some docs / KB article on how to continue with update system by pinning installation to the current version

Yes a KB is in the works.

manasdk commented 8 years ago

@kami

Overall the approach and script looks good to me, but we definitely want automated tests for this

Are you saying do not check-in without automated tests? If so this will atleast push us back by 2-3 days since I suspect there will be a lot of discovery like -

Also, given that we might be moving away from this approach soon is it worth spending more time on this?

Kami commented 8 years ago

@manasdk I think it can live inside st2cd, since it's a workroom test similar to the existing ones.

And yes, I do think it's worth spending time on it (it's rarely not worth spending time on test and "soon" will most likely turn out that we will need to support existing approach for at least one or two months if not more).

manasdk commented 8 years ago

Incredible amount of misery due to my lack of awareness on set -e behavior. I sure hope there is very good reason to use set -e

manasdk commented 8 years ago

Going to merge. Will watch for all hell to break loose!