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 exit code for run command #502

Merged
merged 7 commits into from
Feb 19, 2024
Merged

Fix exit code for run command #502

merged 7 commits into from
Feb 19, 2024

Conversation

adambabik
Copy link
Collaborator

@adambabik adambabik commented Feb 17, 2024

Consider the following Markdown file:

```sh { "name": "test-singleline" }
failhereplease && echo single
```

```sh { "name": "test-multiline" }
failhereplease
echo multi
```

When executing it with runme run, the exit code for both should not be zero. This PR makes sure this is the actual behaviour.

The problem was in how we collect the environment variable. The solution was to switch to a clean up function instead of doing it inline, which could overwrite the original exit status.

Another problem, which is not covered in this PR, is to surface the actual shell exit code rather than any non-zero value convert to 1. This is due to an explicit conversion in:

runme/main.go

Lines 22 to 25 in 064e483

if err := root.Execute(); err != nil {
logf("could not execute command: %v\n", err)
status = 1
}

@adambabik adambabik marked this pull request as ready for review February 17, 2024 15:40
Copy link

sonarcloud bot commented Feb 17, 2024

Copy link
Member

@sourishkrout sourishkrout left a comment

Choose a reason for hiding this comment

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

👍 LGTM. Let's create a follow-up issue for retaining the exit code as described.

@adambabik adambabik merged commit 631b931 into main Feb 19, 2024
6 checks passed
@adambabik adambabik deleted the adamb/fix-exit-code branch February 19, 2024 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants