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

RESOLVED FIXED

Status

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mars, Assigned: mars)

Tracking

Production

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
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
(Assignee)

Comment 2

3 years ago
Created attachment 8765933 [details]
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/
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.
(Assignee)

Comment 4

3 years ago
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/
Attachment #8765933 - Flags: review?(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
Attachment #8765933 - Flags: review?(glob) → review+
Summary: start-local-mozreview and create-test-environment should fail fast if they encounter errors → start-local-mozreview should fail fast if it encounter errors

Comment 6

3 years ago
Pushed by bjones@mozilla.com:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/282cf59890a5
testing: make start-local-mozreview fail on all script errors r=glob
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
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
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.
You need to log in before you can comment on or make changes to this bug.