1279881 - start-local-mozreview should fail fast if it encounter errors

Status: RESOLVED FIXED

(MozReview Graveyard :: Testing / Development Environment, defect)

Description

[Māris Fogels [:mars] (please needinfo)]

When the start-local-mozreview and create-test-environment scripts encounter errors they continue execution, leaving behind a broken dev/test environment.  You also have to scroll back through what could be many many lines of output to find the source of the original error (or use '|& tee' to save the output just in case something goes wrong).

Instead of continuing, the scripts should fail immediately and noisily when they encounter errors.

Comment 1

[:glob ✱]

i'll be landing a patch next week that fixes start-local-mozreview (part of bug 1264203).
https://reviewboard.mozilla.org/r/49613/diff/2#index_header

Comment 2

[Māris Fogels [:mars] (please needinfo)]

Attachment: testing: make start-local-mozreview fail on all script errors (bug 1279881)

Make start-local-mozreview fail on any script error, not just 'mozreview
start'.  Also avoid the much more strict 'set -u' because it doesn't
play nice with scripts that call virtualenv.

Review commit: https://reviewboard.mozilla.org/r/61046/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61046/

Comment 3

[:glob ✱]

https://reviewboard.mozilla.org/r/61046/#review57872

::: ansible/roles/docker-rbweb/files/install-reviewboard:21
(Diff revision 1)
>  lines = open('%s/conf/settings_local.py' % ROOT, 'rb').readlines()
>  with open('%s/conf/settings_local.py' % ROOT, 'wb') as fh:
>      # We would ideally set logging_directory via site config, but Djblets
>      # isn't allowing us to set a string value to a None type. Derp.
>      log_directory_found = False
> +    aws_calling_format_found = False

these changes don't appear to be related to this bug.

Comment 4

[Māris Fogels [:mars] (please needinfo)]

Comment on attachment 8765933 [details]
testing: make start-local-mozreview fail on all script errors (bug 1279881)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61046/diff/1-2/

Comment 5

[:glob ✱]

Comment on attachment 8765933 [details]
testing: make start-local-mozreview fail on all script errors (bug 1279881)

https://reviewboard.mozilla.org/r/61046/#review57892

lgtm

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/282cf59890a5
testing: make start-local-mozreview fail on all script errors r=glob

Comment 7

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/61046/#review58954

::: start-local-mozreview:7
(Diff revision 2)
>  # This script starts the mozreview servers, creates a default user, some bugs
>  # and a properly configured mercurial repository for pushing reviews.
>  # You need to source this script (e.g. run with ". start-local-mozreview") in
>  # order for the environment variables to be set correctly.
>  
> +set -eo pipefail

Heh, we probably should have added a comment when this was removed before[1]. This script is designed to be sourced by your shell, which means any error after this is run will cause your shell to exit on you.

We should really back this out or fix it in a way that avoids the issue.

[1] https://hg.mozilla.org/hgcustom/version-control-tools/rev/060ef94d27b1

Comment 8

[:glob ✱]

https://reviewboard.mozilla.org/r/61046/#review58954

> Heh, we probably should have added a comment when this was removed before[1]. This script is designed to be sourced by your shell, which means any error after this is run will cause your shell to exit on you.
> 
> We should really back this out or fix it in a way that avoids the issue.
> 
> [1] https://hg.mozilla.org/hgcustom/version-control-tools/rev/060ef94d27b1

funny; i've never sourced that file.  i suspect only MOZREVIEW_HOME and |mozreview shellinit| need to be set, which i do separately only once the environment is successfully built.

the file shouldn't be marked as executable if it isn't designed to be executed.