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

Stdout stderr to vec plus error exit codes #50

Merged
merged 3 commits into from
Dec 11, 2023

Conversation

kdesjard
Copy link
Contributor

For your consideration as this pull request also contains breaking changes.

This request allows for the capture of stdout and stderr regardless of whether a task completes successfully. It also allows for the capturing of the OS's exit code in the case of a failure.

Thanks, Kris.

Signed-off-by: kdesjard <kristian.desjardins@nrcan-rncan.gc.ca>
…e in the case of an error and allow for setting a Content payload

Signed-off-by: kdesjard <kristian.desjardins@nrcan-rncan.gc.ca>
…or not. Convert stdout and stderr into a Vec of Strings for downstream use. In the case an error, attempt return the operating systems exit code

Signed-off-by: kdesjard <kristian.desjardins@nrcan-rncan.gc.ca>
@genedna genedna merged commit dd2d5dd into dagrs-dev:main Dec 11, 2023
4 checks passed
@kdesjard kdesjard deleted the stdout_stderr_to_vec branch December 12, 2023 10:54
@@ -165,14 +166,14 @@ impl Output {
}

/// Construct an [`Output`]` with an error message.
pub fn error(msg: String) -> Self {
Self::Err(msg)
pub fn error(code: Option<i32>, msg: String) -> Self {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@kdesjard This PR does not make sense for normal outputs when someone is not running Shell commands. Rust has rich error-handling features, which are much better than exit codes.

Also, this method is changed, which is a breaking change for all the users of Dagrs. We have to revert this change adding a new status field like ErrWithExitCode

@kdesjard
Copy link
Contributor Author

Sorry about this and thanks for the updates/fixes.

@aminya
Copy link
Collaborator

aminya commented Dec 19, 2023

Sorry about this and thanks for the updates/fixes.

You're welcome!

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.

4 participants