Clean / improve install script #46

alexAubin · 2018-02-13T01:24:29Z

This is an attempt to clean and improve the install script.

In particular, for the following :

Simplify the syntax for handling step exception (instead of if ! step foo; then die "bar" fi, just do step foo || die "bar") ;
Centralize 'assertions check', such as being on the right debian distribution, to immediately identify problematic situation before running anything else ;
Wrap everything in function. This is for defensive bash, but also in the optic of moving to a curl | bash instruction, we need to avoid the case where the curl ends up in the middle of the request, but still dumps half the script and we get weirdly corrupted installs ;
Improve message logging cosmetics, and other misc cleaning / reorganization ;
Following some user feedback, detect apache2 and bind9 which might be conflict with nginx and dnsmasq. Provide an option to force the install if the user knows what it's doing ;

As discussed in https://dev.yunohost.org/issues/525 , one of the goal is also to move towards a simple curl https://install.yunohost.org | bash install procedure (and provide more cautious instructions for people willing to check integrity / authenticity..). In particular, we should also be able to move step 3 (= check that root has password - not sure why it's necessary) as a check inside the script.

…erly changed during postinstall

Psycojoker

I've read all the commits, apart the bind9 grep everything looks fine for me (and the "keep old log stuff" but that's less important).

Thanks for this significant amount of work in improving the quality of this code 👍

Psycojoker · 2018-02-13T06:40:00Z

install_yunohost

@@ -210,6 +216,14 @@ function check_assertions()
    if is_raspbian ; then
        user_pi_logged_out || die "The user pi should be logged out."
    fi
+
+    # Check possible conflict with apache, bind9.
+    [[ -z "$(dpkg --list | grep ' bind9')" ]] || [[ "$FORCE" == "1" ]] \


This one will likely failed because it greps every package that has "bind9" in its name and YunoHost itself requires bind9-utils (for dyndns-keygen used by TSIG). You probably want to do a grep 'bind9 ' (with a space).

Indeed, not sure why but actually in the final diff it's with bind9 (which is probably why github is flagging as outdated) so that should be ok :P

Psycojoker · 2018-02-13T06:41:51Z

install_yunohost

-    parse_options
+    parse_options "$@"
+
+    rm -f $YUNOHOST_LOG


You might want to do a mv + date $YUNOHOST_LOG $YUNOHOST_LOG$(date +%F_%X) instead to keep old logs for debugging.

Yea, I was a bit lazy about that but that shouldn't take long to implement, will do x)

okay, once it's done you can merge for me :)

Psycojoker · 2018-02-13T06:44:07Z

Ah, so apparently github haven't kept my bind9 comment :|

Psycojoker

Review system seems broken when made on several commits.

Psycojoker · 2018-02-13T06:44:43Z

install_yunohost

@@ -210,6 +216,14 @@ function check_assertions()
    if is_raspbian ; then
        user_pi_logged_out || die "The user pi should be logged out."
    fi
+
+    # Check possible conflict with apache, bind9.
+    [[ -z "$(dpkg --list | grep ' bind9')" ]] || [[ "$FORCE" == "1" ]] \


This one will likely failed because it greps every package that has "bind9" in its name and YunoHost itself requires bind9-utils (for dyndns-keygen used by TSIG). You probably want to do a grep 'bind9 ' (with a space).

Psycojoker · 2018-02-13T06:46:20Z

Also thanks a lot for having taken the time to split this in a lot of commits, this has made the review painless for me and I greatly appreciate it ❤️

alexAubin · 2018-02-26T18:29:19Z

Fixed the remaining point and tested an install on a VPS with wget | bash: it seems to be working as expected.. (Except that at the end, when attempting to launch the postinstall, it failed, but then launching it manually worked ?)

Psycojoker · 2018-02-28T08:46:33Z

Except that at the end, when attempting to launch the postinstall, it failed, but then launching it manually worked ?

That looks weird, but shouldn't we fix that before merging :o?

alexAubin · 2018-02-28T08:57:09Z

I guess x) I'll have a deeper look

alexAubin · 2018-03-06T14:36:36Z

So after investigating :

this is actually not an issue with the script, but with the way I used it ...
turns out that you cant use curl | bash if you expect the script to use stdin, which we do when we run the postinstall (ask the main domain, then password)
as described in this stackoverflow thread, an alternative is to use bash <(curl https://url/of/script.sh), which is a bit less elegant...

So, not sure what to do with this... I see those possibilities :

keep it as it is right now, but we'll have tell people to either use curl <(bash), or something like curl && source script.sh ;
ask for main domain and password with whiptail ;
don't attempt to run the postinstall from the install script, just tell the user to run yunohost tools postinstall.

alexAubin · 2018-03-08T15:56:25Z

Okay I'm merging this as is, since as described, the issue I mentioned is not related to the changes but with the way I did my tests. We can discuss at some point how we want to design the use of this script.

Alexandre Aubin added 14 commits February 12, 2018 23:15

Copyright year / project name

1c30817

Add function prefix to functions ...

d3055fb

Simplify exception handling for steps

370a12e

Wrap option parsing and main into functions

a3f589d

Reorganize functions + misc stuff

719bbf1

This specific metronome stuff for raspbian is not necessary anymore

23c124a

Dunno why we need this for Raspbian specifically ... hostname is prop…

0c89145

…erly changed during postinstall

Have a function to check install script assertions before anything else

b0f680e

Snake case to be consistent with other functions

a3ed563

More cosmetic / reorganizing / misc cleaning

6cbe6f5

Rework message / logging system

caef251

Check for possible conflicts with apache, bind9

b12fbc9

Cosmetics

89f4ffe

Fixes following tests

ec297b8

alexAubin added small decision opinion needed labels Feb 13, 2018

Psycojoker requested changes Feb 13, 2018

View reviewed changes

Use datetime in install log file name

d04c67e

alexAubin merged commit 882ee11 into stretch2 Mar 8, 2018

alexAubin deleted the improve-install-script branch March 8, 2018 16:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean / improve install script #46

Clean / improve install script #46

alexAubin commented Feb 13, 2018 •

edited

Psycojoker left a comment

Psycojoker Feb 13, 2018

Psycojoker Feb 13, 2018

alexAubin Feb 13, 2018

Psycojoker Feb 13, 2018

alexAubin Feb 13, 2018

Psycojoker Feb 13, 2018

Psycojoker commented Feb 13, 2018

Psycojoker left a comment

Psycojoker Feb 13, 2018

Psycojoker commented Feb 13, 2018

alexAubin commented Feb 26, 2018

Psycojoker commented Feb 28, 2018

alexAubin commented Feb 28, 2018

alexAubin commented Mar 6, 2018

alexAubin commented Mar 8, 2018

Clean / improve install script #46

Clean / improve install script #46

Conversation

alexAubin commented Feb 13, 2018 • edited

Psycojoker left a comment

Choose a reason for hiding this comment

Psycojoker Feb 13, 2018

Choose a reason for hiding this comment

Psycojoker Feb 13, 2018

Choose a reason for hiding this comment

alexAubin Feb 13, 2018

Choose a reason for hiding this comment

Psycojoker Feb 13, 2018

Choose a reason for hiding this comment

alexAubin Feb 13, 2018

Choose a reason for hiding this comment

Psycojoker Feb 13, 2018

Choose a reason for hiding this comment

Psycojoker commented Feb 13, 2018

Psycojoker left a comment

Choose a reason for hiding this comment

Psycojoker Feb 13, 2018

Choose a reason for hiding this comment

Psycojoker commented Feb 13, 2018

alexAubin commented Feb 26, 2018

Psycojoker commented Feb 28, 2018

alexAubin commented Feb 28, 2018

alexAubin commented Mar 6, 2018

alexAubin commented Mar 8, 2018

alexAubin commented Feb 13, 2018 •

edited