Increase code quality

[ypid]

shellcheck is very nice for showing you possible problems in shell scripts.

Todos I noticed:

• Quote all variables in bash

Refs: techandme/owncloud-vm#6 (comment)

EDIT: Please read this: #20 (comment)

[enoch85]

@ypid Started a PR here: #21 Feel free to help.

[WaaromZoMoeilijk]

Ran the install on a fresh vm, no problems installing. Only the documents app...

nextcloudvminstalllog.txt

[enoch85]

Great! Yeah, the documents app fails. Please comment it out in this #21 PR as well.

I will run the patch later and test that everything works out. Great job @ezraholm50!

[enoch85]

Btw, now @nextcloud/vm is available. FYI. :)

[WaaromZoMoeilijk]

@enoch85 Done! Better to have us both check it indeed. I'll run it another time somewhere tonight if i can manage to get some time off.

Oh wow nice one at the VM team!

[enoch85]

All the files in 'static' aren't checked yet... ;)

[WaaromZoMoeilijk]

Actually I think they are, I git cloned the entire ShellCheck branch, moved all the scripts to /var/scripts

And I ran this just to be sure.
sed -i 's|production repo link|shellcheck repo link|g' /var/scripts/*

So that to me feels like everything that has been changed works.

Or did you mean that you didn't had the chance to change them?
@enoch85

[enoch85]

@ezraholm50 What I mean is that you should upload your changes to Github, the spellcheck branch. Please check for yourself, the only change I can see is that you hashed out the Documents app. :S

[WaaromZoMoeilijk]

I thought you pushed all the changes to the branch, haven't checked all the files in the pull req by view. So the entire /static still needs to be changed then? That will take a little while, no problem.

[enoch85]

@ypid PR failed. We followed the instructions from ShellCheck. These scripts are built over time for the past 2 years now. We have tested everything thoroughly over time, but with ShellCheck it fails .

Feel free to open a PR if you want.

[ypid]

ShellCheck failed? It just gives suggestions 😉 Maybe you can start small. Run Shellcheck and then check the description of each issue.

Each issue has an ID with which you can visit the wiki of Shellcheck:
Example: https://github.com/koalaman/shellcheck/wiki/SC2086

[WaaromZoMoeilijk]

We did, most of the suggestions work, though not all. And to be honest with about 10% I really wouldn't know what to change.

This is put on long term, @enoch85? My idea was also to start small with some double quote's and few other things that work.

Thanks for the suggestion anyway!

[enoch85]

@ypid @ezraholm50 This is opensource, anyone can contribute ;) PRs are more than welcome.

[enoch85]

Why did you close @ezraholm50?

[enoch85]

Ref #36

[enoch85]

@ezraholm50 I still don't get why you closed this? It's long term, and it doesn't hurt if someone else wants to take care of this - I reopen.

[enoch85]

The code works, but doesn't look good. It needs to be consistent and properly intendented to please the eye. Stuff like:
command
if [ $? -eq 1 ]; then ...
Could be replaced with the if argument directly instead, like so:
if command; then ...

Also use elif where possible to produce more code on fewer lines.

Except that the code is not consistent. In some places we use > instead of -gt which is the proper way of doing it.

And then we have... intendeation(!!) We should use 4 spaces in a if argument for example. This is a good example.

We also have this in many places:

   echo -e "\e[32m"
   read -p "Press any key to continue... " -n1 -s
   echo -e "\e[0m"

Which could be done on the same line.

There are a lot of other stuff I can't think of now, but anyone who have some time left over could do this one step at the time. My suggestion is to start with the smaller scripts and then work our way up to the main scripts (installation & setup).

There is a personal bounty for the person/persons who does this in one sweep (everything at once), meaning if someone decides to spend time on this, create a PR and do everything according to my suggestions (or better) that person (or group of people) will be rewarded with $150 (total amount) on their PayPal account when I (together with maintainers of this repo) have approved the changes. Please ask if you have any questions regarding the reward/bounty.

I posted a project that you can use for this if you want, it's found here: https://github.com/nextcloud/vm/projects/1

Let's get this done! cc @nextcloud/vm

[morph027]

As bash-fetishist, i'll do this, did this for other projects too. If i'll be first one to pass review, money can be donated to fsfe ;)

[morph027]

List of scripts...

• ./nextcloud_update.sh
• ./nextcloud_install_production.sh
• ./static/ip.sh
• ./nextcloud-startup-script.sh
• ./static/trusted.sh
• ./static/instruction.sh
• ./static/ip2.sh
• ./static/ntpdate.sh
• ./static/security.sh
• ./static/passman.sh
• ./static/temporary-fix.sh
• ./static/collabora.sh
• ./static/history.sh
• ./static/spreedme.sh
• ./static/change-ncadmin-profile.sh
• ./static/nextant.sh
• ./static/phpmyadmin_install_ubuntu16.sh
• ./static/update.sh
• ./static/redis-server-ubuntu16.sh
• ./static/nextcloud.sh
• ./static/change-root-profile.sh
• ./static/setup_secure_permissions_nextcloud.sh
• ./static/change_mysql_pass.sh
• ./static/test_connection.sh
• ./static/adduser.sh
• ./lets-encrypt/test-new-config.sh
• ./lets-encrypt/activate-ssl.sh

[enoch85]

@morph027 Thank you! If you find anything that could be improved even more let's do it. I can't remember all the thousands of lines right now. :)

And please ask me if you are wondering about any functions.

[morph027]

Ok...let's start ;)

Why do we sometimes just ask to press any key to do something? What if the person does not want to take the action?

[morph027]

This is unclear to me:

sudo passwd "$UNIXUSER"
if [[ $? -gt 0 ]]
then
    sudo passwd "$UNIXUSER"
else
    sleep 2
fi

If it fails one time, it can fail a second time ;) If we really need to change the password, we should do this prompt in a loop until it was successful.

Same here:

sudo -u www-data php "$NCPATH/occ" user:resetpassword "$NCADMIN"
if [[ $? -gt 0 ]]
then
    sudo -u www-data php "$NCPATH/occ" user:resetpassword "$NCADMIN"
else
    sleep 2
fi

[enoch85]

Yes, that's a great example of old code that needs to be upgraded. A loop would be much better - the intention is to give the user another chance to type the correct password if it fails, and this was one of the first things I wrote some years ago.

So please, make it a loop until the attempt is successful. Thank you!

[enoch85]

Why do we sometimes just ask to press any key to do something? What if the person does not want to take the action?

This is becuase in some cases there are information that could be useful to the user, and if he/she wouldn't press a key to continue he/she wouldn't have a chance to read what happened. Like for example; it's good to know if something has failed. Do you have a better suggestion?

[morph027]

This is becuase in some cases there are information that could be useful to the user, and if he/she wouldn't press a key to continue he/she wouldn't have a chance to read what happened. Do you have a better suggestion?

No, fine...just for better understanding. Just in case of multiple runs, the user probably does not want to adjust the timezone every time ;) But this could be fixed with some markers, which indicates, that a section succeeded and we can skip it.

[morph027]

By the way...do you really want to review one HUUUUGE pull request? Just for reviewing purposes, it might be better to make small potions (e.g. for each script) to have atomic discussions in places, where it belongs.

[enoch85]

The script isn't intended to be run several times (nextcloud-startup-script.sh) and assumes certain things (don't remember everything now) but if we could make it safe to run several times that would be awesome. Like check if things are already setup and in that case skip them - 👍 💯 from me on that. In later coding I have thought of making it possible to run several times, so the code is kind of mixed with the old where it would fail hard if that happened.

Some things I remember from the top of my head;

• It set's a new mysql password and then deletes the password file in the phpmyadmin installation, on a second run that would fail hard. (Btw, make phpMyadmin installation optional and come up with a better way of fetching the needed password for the installation. The current password should always be in .my.cnf, don't remember why I kept mysql_password.txt..)
• The UTF8 convert isn't supposed to be run twice. Maybe check if that's already done and skip if it is..?

By the way...do you really want to review one HUUUUGE pull request?

As everything is tightly connected to eachother (some scripts depends on others etc, it's crucial that everything is perfect, and in a HUGE PR we could test that branch in one go on a test VM for example... So yes, I prefer a HUGE PR. :) I'm ofc counting with that you test all the scripts together and separately as well to be sure everything is bug free. :)

[enoch85]

@morph027 BTW, join #techandme on IRC if you want to discuss there instead. :)

Also, regarding the PRs, you could commit to your repos master and I can check it there before you make a PR out of it.

[enoch85]

Oh, and btw. On this was on my todo list as well, maybe you can check why it happens sometimes?

[enoch85]

to have atomic discussions

We could do that in IRC or in your repo.

[enoch85]

For reference: https://github.com/morph027/vm/tree/20-increase-code-quality

[PietsHost]

I would like to use variables for colored messages.. like this:

green='\e[32m'
reset='\e[0m'
[.... some code ....]
printf $green"here's my text"$reset

instead of

echo -e "\e[32m"
   read -p "Press any key to continue... " -n1 -s
   echo -e "\e[0m"

and also use printf instead of "echo -e", as described in the APPLICATION USAGE section of the following spec, for reliably portable code: POSIX specification for echo

[PietsHost]

Ah I've got another hint, bc I've just seen your variables:
All-caps environment variable names are actually reserved by POSIX-specified convention for variables with meaning to the shell or operating system, whereas lowercase names are reserved for application use. (Even if you aren't exporting your own variables to the environment, setting a shell variable overwrites any like-named environment variable, making the convention necessarily apply in both places)

[enoch85]

@PietsHost Hi there! :) So nice of you to contribute! :)

Right now we are working in @morph027 repo. Add you sugesstions there as a PR and we can discuss it.

Thank you!

[enoch85]

@@PietsHost Also, you did some great stuff with prigress meters in you modified scripts. Would be nice to see them here as well. 👍

[PietsHost]

Okay :)

[enoch85]

@morph027 @PietsHost How far have we come with the scripts?

[PietsHost]

No updates today, sorry.
I'm really busy right now.. maybe tomorrow

[morph027]

Same here..did some minor stuff but will have some more time tomorrow ;)

[morph027]

Ok....according to my list, every script in my branch is shellcheck'ed at least...

[enoch85]

@morph027 OK, nice! Good job! 👍

Now, let's improve the code in general. Things like removing unnecessary lines, compress if arguments, add functions and improve how the code works. You had some smart stuff going on with your while loops, and I think they can be used in more places as we discussed earlier.

I'm on IRC if you want to discuss.

[enoch85]

All if arguments are checked. Left some comments for you @morph027 to improve even more. Also, please do a sed on the file checking and dir checking. I did if ! [ -f ... while it should be if [ ! -f ...

[enoch85]

https://github.com/morph027/vm/pull/13#issuecomment-291194697 @morph027

[enoch85]

@morph027 wrote:

As bash-fetishist, i'll do this, did this for other projects too. If i'll be first one to pass review, money can be donated to fsfe ;)

Rewrite is almost done (only 2 errors left afaik), just donated $149.71 to FSFE as @morph027 and me did everything ourselves. @PietsHost contributed with one commit but left the branch after that. Total commits will be around ~300. The code is improved very much and now Travis is included to be sure that it stays that way.

Thank you for your good work and ideas @morph027, and thanks @PietsHost for your commit.

[ypid]

Awesome work guys, thanks!

[morph027]

❤️

Great....will still continue looking at the code if my brain has some spare time left ;)