Move to set -e everywhere

[danpoltawski]

Any sort of shell scripts which execute and don't stop on errors can become incredibly dangerous.

We should move to activating set -e everywhere. It will be a pain, but its always worth it. Please ensure any new files start with set -e as it will make it easier in future.

[danpoltawski]

e.g. When i see things like this, it gives me a heart attack ;-)

./compare.sh
rm: /srv/moodleperf/datadir/*: No such file or directory
rm: -rf: No such file or directory
ERROR:  database "moodleperformancecomparison" does not exist
ERROR:  permission denied to create database

[dmonllao]

If we set -e there will not be any custom-hypercool error message and users will need to know more internals to know where the problem comes from, which means that everybody that finds a small problem (most of the people as there are a few config vars) will have to debug the shell scripts. The tool was tested (and also developed in the last stages) with set -e and it should all work with set -e (not all the cases have been tested as this was not spotted) IMO we should spot all those cases during development and testing and provide nice error messages for people.

set -e can be used to debug unknown problems (https://github.com/moodlehq/moodle-performance-comparison/blob/master/README.md#troubleshooting) that rm is as dangerous as it is with or without set -e.

I would write a patch to fix that rm when $dataroot has no contents.

[danpoltawski]

I disagree - we handle the errors, the errors don't get through to the script, and if an error occurs that the we haven't considered, then we need to stop, because without set -e, your expectations aren't met.. maybe you expect a file to be there or not and then the script as though everything worked perfectly.

This is super dangerous IMO. Its even more confusing for the user, because half the operations might not have happened, and the script continues and they need to work out why.

(And yep about the rm, I already have a patch for that).

[dmonllao]

Ok, how can we provide useful feedback messages when errors occurs using set -e?

[danpoltawski]

You simply detect the error and handle it. Set -e just stops the script from continuing with unhanded errors, and those are the errors which are really dangerous because they are unknown!

For example:

#!/bin/bash

set -e
echo "Loading stuff from config file.."
cat configfile || echo 'Config file not found'; exit 1
echo "Continuining to do dangerous stuff based on the contents of the config file."

From the bash man page:

              -e      Exit immediately if a simple command (see SHELL GRAMMAR above) exits with a non-zero status.  The shell does not exit
                      if  the  command  that  fails is part of the command list immediately following a while or until keyword, part of the
                      test in an if statement, part of a && or || list, or if the command's return value is being inverted via !.   A  trap
                      on ERR, if set, is executed before the shell exits.

[dmonllao]

This is ready to integrate in the 3 branches: https://github.com/dmonllao/moodle-performance-comparison/compare/moodlehq:master...issue_36

Includes:

• set -e
• delete_files with an optional argument to check that the specified file/dir/path/list_of_files exists
• A small minimum initial test

[danpoltawski]

Hi David,

Haven't reviewed properly yet, but on a quick glance, I think that it would be better for readability to split some of the lines across multiple lines with backslashes.

(I am not sure of the best style, but just as an example)

${phpcmd} admin/cli/install_database.php \
                --agree-license \ 
                --fullname="$sitefullname" \
                --shortname="$siteshortname" \
                --adminuser="$siteadminusername" \
                --adminpass="$siteadminpassword" > /dev/null \
                || throw_error "Moodle can not be installed, check that the database data is correctly set"

(Actually I read about google style guide the other day, maybe we could look at that http://google-styleguide.googlecode.com/svn/trunk/shell.xml )

[dmonllao]

I think this has nothing to do with this issue, but I will add a commit here

[danpoltawski]

Perhaps, but you are adding even longer lines (and actions) be adding the || at the end of already very long lines, I can see it becoming unmanageable.

[dmonllao]

I've uupdated the https://github.com/dmonllao/moodle-performance-comparison/compare/moodlehq:master...issue_36 branch including a last commit to improve a few things according to the style guide.

[dmonllao]

Hi Dan,

Any chance you can review this before integrating it?

[danpoltawski]

Hi David,

I just looked at this and it looks like a great improvement to me.

• The syntax error in comment we spoke about in b48f9c880.
• Line length - I see you've moved to google's 80 char limit in some places, but not everywhere. I don't feel strongly in favour of 80 chars. But it seems a shame to split up line 81 of after_run_setup.sh to fit within the limit and then have longer lines elsewhere.

[dmonllao]

Thanks for the review Dan, I haven't restricted the char limit to 80, I split very long lines in logical parts (CLI command options, command + throw_error()...) I've now reviewed all .sh files and I've split a couple of long error messages in multiple lines, so all lines have less than 100 chars (approximately) I've corrected the > &2 too, thanks, merging.

[dmonllao]

merged in 25, 26 and master