Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Change heroku run -x implementation to use a Bash EXIT trap #2468

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Change heroku run -x implementation to use a Bash EXIT trap #2468

wants to merge 2 commits into from

Commits on Sep 1, 2023

  1. Use EXIT trap for heroku-command-exit-status 

    Appending our own `; echo …` to the command has several drawbacks:

- it does not run if the command contains an `exit` statement (simple example: `heroku run -x 'exit 3'`);
- it does not run if the shell itself exits, e.g. on commands that use `set -e` or `set -u`;
- it breaks when the preceding command uses valid syntax that conflicts with the suffix (simple example: `heroku run -x 'echo hello;'`);
- it causes the wrong exit status to be reported in `heroku logs` - it's always 0 (from the `echo`), not the correct value.

Using a `trap` on `EXIT` instead works around all of these issues.
    @dzuelke
    dzuelke committed Sep 1, 2023
    Configuration menu
    Copy the full SHA
    c19da44 View commit details
    Browse the repository at this point in the history

  2. Fix heroku-command-exit-status newline handling 

    The match call uses multiline mode for no reason (it contains no anchors).

The replace call anchors at the start of the string, but this is not a guarantee, and on failure, the magic string is visible in the output (simple example: `heroku run -x 'echo "hi"; echo -n "yo"'`).

Solution is to remove the leading anchor.

It also leaves behind a stray newline after replacing - the reason is that the `\n?` is ungreedy due to the `?`, so it will not actually match the newline character. The result is that using `heroku run -x` produces an extra blank line at the end of the output.

Solution is to not have the newline optional, or to use a different trailing marker - I have re-used `\uFFFF` here, because that also allows removing multiline mode.

Ideally, we'd use a proper unique string - e.g. generate a GUID for the command, and match that exact GUID later. Could also use a different separator - `\uFFFF` is not legal unicode, but 💜 is ;) (that's `\U1F49C` in Bash, with uppercase "`U`", and `\uD83D\uDC9C` in JavaScript, expressed as a UTF-16 surrogate pair).
    @dzuelke
    dzuelke committed Sep 1, 2023
    Configuration menu
    Copy the full SHA
    6cfffa8 View commit details
    Browse the repository at this point in the history