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.

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

fix(pwsh): improving pwsh error handling #4504

Merged
merged 3 commits into from
Oct 9, 2024
Merged

Conversation

alpetric
Copy link
Contributor

@alpetric alpetric commented Oct 8, 2024

Unix & Windows

Currently pwsh scripts will not return an error exit code if a cmdlet fails in a "non-terminating" way. Error handling in pwsh is quite complex and different from bash.

The implemented behavior wraps the user provided script in a:

$ErrorActionPreference = 'Stop'
Set-StrictMode -Version Latest

try {

     # User Provided Code

} catch {
    Write-Output "An error occurred:\n"
    Write-Output $_
    exit 1;
}

Windows

exit $LASTEXITCODE in wrapper.ps1 will propagate error code to windmill


Important

Improves PowerShell error handling in bash_executor.rs by wrapping scripts in a try-catch block and setting strict error preferences.

  • Behavior:
    • Wraps PowerShell scripts in a try-catch block with $ErrorActionPreference = 'Stop' and Set-StrictMode -Version Latest to catch non-terminating errors.
    • On error, outputs error message and exits with code 1.
  • Files:
    • Updates bash_executor.rs to include error handling logic for PowerShell scripts.
    • Modifies wrapper.ps1 to propagate error code using exit $LASTEXITCODE.

This description was created by Ellipsis for d16241c. It will automatically update as commits are pushed.

Copy link

cloudflare-workers-and-pages bot commented Oct 8, 2024

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: d31dbb8
Status: ✅  Deploy successful!
Preview URL: https://fb8340dc.windmill.pages.dev
Branch Preview URL: https://alp-pwsh-error-handling.windmill.pages.dev

View logs

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to d16241c in 17 seconds

More details
  • Looked at 50 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. backend/windmill-worker/src/bash_executor.rs:332
  • Draft comment:
    Consider using Write-Error instead of Write-Output for error messages to better align with PowerShell's error handling practices.
            Write-Error "An error occurred:"
            Write-Error $_
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR introduces error handling for PowerShell scripts by wrapping user code in a try-catch block. However, the catch block uses Write-Output which might not be the best choice for error messages. Write-Error is more appropriate for error messages in PowerShell.
2. backend/windmill-worker/src/bash_executor.rs:334
  • Draft comment:
    Consider allowing users to specify a custom exit code or message in the catch block for more flexibility in error handling.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR adds a try-catch block for error handling in PowerShell scripts. However, the exit 1 in the catch block might not be the best approach for all scenarios. It could be beneficial to allow the user to specify a custom exit code or message.
3. backend/windmill-worker/src/bash_executor.rs:373
  • Draft comment:
    Ensure that $LASTEXITCODE is set correctly before using it in exit $LASTEXITCODE. If the script does not set it, it might not reflect the intended exit code.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR adds a try-catch block for error handling in PowerShell scripts. However, the exit 1 in the catch block might not be the best approach for all scenarios. It could be beneficial to allow the user to specify a custom exit code or message.

Workflow ID: wflow_F3FsRP6akRWuGpGG


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@rubenfiszel rubenfiszel changed the title improving pwsh error handling fix(pwsh): improving pwsh error handling Oct 9, 2024
@rubenfiszel rubenfiszel merged commit f831b9b into main Oct 9, 2024
3 checks passed
@rubenfiszel rubenfiszel deleted the alp/pwsh_error_handling branch October 9, 2024 03:20
@github-actions github-actions bot locked and limited conversation to collaborators Oct 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants