[TRAVIS] EZEE-2236: Retry installation after Composer failure

[mnocon]

Question	Answer
JIRA issue	Done as part of https://jira.ez.no/browse/EZEE-2236
Bug/Improvement	CI improvment
New feature	no
Target version	6.7, 6.13, 7.2, master
BC breaks	no
Tests pass	no
Doc needed	no

Sometimes composer install fails because of network connection problems, for example:

```[37;41m                                                                               [39;49m
[37;41m  [Composer\Downloader\TransportException]                                     [39;49m
[37;41m  The "http://packagist.org/p/ezsystems/ezpublish%24668516c3c81fe7235019645bb  [39;49m
[37;41m  ea5df277a1823a31351e9a2b6dd701bbfbb404e.json" file could not be downloaded   [39;49m
[37;41m  (HTTP/1.1 404 Not Found)                                                     [39;49m
[37;41m                                                                               [39;49m```

Adding travis_retry should help avoid this (and improve stability).

TODO:

• Implement feature / fix a bug.
• Ask for Code Review.

[alongosz]

Are you sure this is going to work? It depends on script return code, and prepare_unittest.sh will always return 0, even if there are errors in subcommands. Error code from subcommand is not propagated.

[mnocon]

Good catch, I forgot about the "exit code is the code of last executed command" bit 😞

As I see it, there are two options here:

1. use set -e in prepare_unittest.sh
2. check the return value of each composer command and exit with it if it's non-0.

WDYT?

[andrerom]

1

[andrerom]

BUT, several things in that script should ideally not be done several times. So this might cause some super strange issues down the line...

[alongosz]

BUT, several things in that script should ideally not be done several times. So this might cause some super strange issues down the line...

That's a good point. So we should use it directly for composer install/update

@mnocon investigated it further and it looks like we cannot use travis_retry inside a script.

I think we need to refactor prepare_unittest.sh and move composer install/update outside, directly to install section of .travis.yml. Then we would be able to use travis_retry.

[mnocon]

Good catch Andre, I haven't thought about it.

I'll try the travis_retry inside the script approach (my knowledge is based on https://twitter.com/plexus/status/499194992632811520 - I haven't found anything about both supporting and not supporting it in scripts in official doc, so it's fastest way to check).

If it fails I will do the refactoring with moving the composer bits (and the ones after it) into the .travis.yml file.

[andrerom]

I can confirm it does not work inside scripts like @alongosz says, might be a way, but I'm in favor of refactoring it to do it outside, slight problem becomes the install vs update problem, but if we are sure we never have a lock (script can remove it if it is there in case update is needed for instance) then we can always run install.

[mnocon]

@alongosz I've pushed another changes. I've decided to add the travis_retry function and use it directly in the script.

I know we've talked about extracting the composer parts - I tried this approach and I didn't like it.

This approach is much clearer IMHO - the whole logic of setting up unit tests is still in one place. Script prepare_unittest sets two variables: COMPOSER_UPDATE and COMPOSER_ROOT_VERSION, which are not available in .travis.yml. I'd have to copy the logic from the script to the Travis file to determine their value again (all of this must be combined together with conditions that make sure we're setting unit tests, not running Behat), it's not readable to me.

I feel like extracting it just for the purpose of using travis_retry is an overkill (maybe there are other reasons to refactor, but I'm not seeing them) and this new approach is better.

What do you think?

[alongosz]

I don't want to maintain travis_retry ;)

Current changes with more logic moved to .travis.yml, while maybe make it a little bit "packed", but seem to me like a good approach.

[mnocon]

@alongosz do you think we can merge this one? We've decided to wait because of temporary Travis issues, but it's green now

[alongosz]

Fine by me. Final review ping @andrerom

[andrerom]

besides nitpic on possible simpliciation, +1

[andrerom]

Did you try without this?
Afaik this has since been resolved, so cool if you can try removing those two lines in a commit and see if all jobs still pass.

[mnocon]

Thanks @andrerom , it works without it 🎉
I've squashed all commits into one, as it became a mess over time.

[alongosz]

Looks even better!