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

return non-zero error code when function run sees error #401

Merged
merged 2 commits into from
Dec 18, 2024
Merged

return non-zero error code when function run sees error #401

merged 2 commits into from
Dec 18, 2024

Conversation

jacobsteves
Copy link
Member

@jacobsteves jacobsteves commented Dec 11, 2024 •
edited
Loading

We want to support returning a non-zero error code anytime a module sees an error during a run.

Heres an example output:

 ✘  ~/src/github.com/Shopify/function-runner   js.return-error-when-module-errors  function-runner -f 'panic-configurable-fuel.wasm' -i 'input.json' -e run
            Input
{ manually truncated }
            Logs

thread '<unnamed>' panicked at src/run.rs:24:5:
oh no - something terrible has happened
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
error while executing at wasm backtrace:
    0: 0x17c4e - <unknown>!<wasm function 366>
    1: 0x84a9 - <unknown>!<wasm function 14>
    2: 0x13f68 - <unknown>!<wasm function 108>
    3: 0x17bf8 - <unknown>!<wasm function 355>
    4: 0x171b0 - <unknown>!<wasm function 243>
    5: 0x17182 - <unknown>!<wasm function 242>
    6: 0x57b8 - <unknown>!<wasm function 9>

        Invalid Output


         JSON Error

EOF while parsing a value at line 1 column 0

        Resource Limits


Input Size: 125.00KB
Output Size: 19.53KB
Instructions: 11M


     Benchmark Results

Name: panic-configurable-fuel.wasm
Linear Memory Usage: 1088KB
Instructions: 150.943K
Input Size: 4.98KB
Output Size: 0B
Module Size: 112KB


Error: The Function execution failed. Review the logs for more information.
 ✘  ~/src/github.com/Shopify/function-runner   js.return-error-when-module-errors  echo $?
1

jacobsteves
jacobsteves commented Dec 11, 2024
View reviewed changes
src/main.rs Outdated Show resolved Hide resolved
@andrewhassan
Copy link
Contributor

I think we stopped auto-updating in this PR, which was released in the 3.65.0 version on August 2. I wonder if that's enough time that it would be okay to ship this PR as the default behaviour.

@nickwesselman Do you have an opinion on this?

jeffcharles
jeffcharles approved these changes Dec 11, 2024
View reviewed changes
Copy link
Contributor

@jeffcharles jeffcharles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we make a case for a breaking change for older versions of the Shopify CLI here? I would imagine we would want older versions of the CLI to error out in these cases. But if not, this seems fine.

src/main.rs Outdated Show resolved Hide resolved
src/engine.rs Outdated Show resolved Hide resolved
src/main.rs Outdated

/// Return a non-zero exit code when a module error occurs during the function run.
#[clap(short = 'n', long)]
non_zero_exit_code_for_module_errors: bool,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a pretty long flag name to me. Unfortunately I don't have a constructive suggestion for something more succinct.

@jacobsteves
Copy link
Member Author

Could we make a case for a breaking change for older versions of the Shopify CLI here?

I think it could be fair to make a case on that. It would make the changes here much nicer. I'll wait to see if Nick has an opinion there.

@jacobsteves jacobsteves force-pushed the js.return-error-when-module-errors branch from fcde9f1 to a59c72b Compare December 11, 2024 22:09
@jacobsteves
optionally return non-zero error code when function run sees error
9284dcf
@jacobsteves
replace exit_code test fixture with noop test fixture when tests rely…
41d5220 
… on success results

The exit code function does not return valid JSON which means it will fail with a non-zero exit code.
Instead, use a new noop test fixture that does return valid JSON for tests that expect a successful result.
@jacobsteves jacobsteves force-pushed the js.return-error-when-module-errors branch from a59c72b to 41d5220 Compare December 17, 2024 21:56
@jacobsteves
Copy link
Member Author

I got confirmation from Nick that we can release this with a changelog without needing a command line option. I'll release a new 7.0 version once this ships.

As a side effect of removing the option, some integration tests started failing because the test fixture produced an invalid result. Thats been fixed by this commit.

jeffcharles
jeffcharles approved these changes Dec 17, 2024
View reviewed changes
Copy link
Contributor

@jeffcharles jeffcharles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥇

@jacobsteves jacobsteves merged commit 8de2185 into main Dec 18, 2024
5 checks passed
@jacobsteves jacobsteves deleted the js.return-error-when-module-errors branch December 18, 2024 14:19
@jacobsteves jacobsteves mentioned this pull request Dec 18, 2024
Merged
@jacobsteves jacobsteves changed the title optionally return non-zero error code when function run sees error return non-zero error code when function run sees error Dec 18, 2024
@jacobsteves jacobsteves mentioned this pull request Dec 18, 2024
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeffcharles jeffcharles jeffcharles approved these changes

@andrewhassan andrewhassan Awaiting requested review from andrewhassan

@mssalemi mssalemi Awaiting requested review from mssalemi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jacobsteves @andrewhassan @jeffcharles