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

Feature Request: Exec must return a struct that contains exit code, stdout, stderr #617

Closed
Warchant opened this issue Apr 29, 2024 · 12 comments · Fixed by #622 or #631
Closed

Feature Request: Exec must return a struct that contains exit code, stdout, stderr #617

Warchant opened this issue Apr 29, 2024 · 12 comments · Fixed by #622 or #631
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@Warchant
Copy link

Current exec has this signature pub async fn exec(&self, cmd: ExecCommand) {. It returns nothing.

In my tests I want to run a command and process resulting exit code & logs (both stderr & stdout).

Is it possible to add support for this feature?

@Warchant Warchant changed the title Exec must return Result with logs & exit code Exec must return a struct that contains exit code, stdout, stderr Apr 29, 2024
@Warchant Warchant changed the title Exec must return a struct that contains exit code, stdout, stderr Feature Request: Exec must return a struct that contains exit code, stdout, stderr Apr 29, 2024
@DDtKey
Copy link
Collaborator

DDtKey commented Apr 29, 2024

Hi @Warchant,

We currently provide the ability to pass WaitFor with ExecCommand, so you can ensure that the command output contains the required log lines (either stdout or stderr, but not both at the moment). For example:

let cmd = ExecCommand::new(...).with_cmd_ready_condition(WaitFor::message_on_stdout(".."));

But I understand the benefit of being able to get all the logs and execution code.
This is possible, but we need to expand the functionality of the logs - split the stream into two.

@DDtKey DDtKey added the enhancement New feature or request label Apr 29, 2024
@DDtKey
Copy link
Collaborator

DDtKey commented Apr 29, 2024

Another good point is that we ideally need to provide the logs as a stream of bytes, since the logs may not be valid UTF-8.

@Warchant
Copy link
Author

Warchant commented Apr 30, 2024

@DDtKey for my use-case it is not really necessary to access stdout & stderr separately. I see that bollard API does not allow to easily separate those. Maybe first version can just return a single stream?
I think

attach_stdout: Some(stdout),
attach_stderr: Some(stderr),
here we can pass both attach_stdout: Some(true) and attach_stderr: Some(true) and then just return output: Pin<Box<dyn Stream<Item = Result<LogOutput, Error>> + Send>>, to the user.

@Warchant Warchant mentioned this issue Apr 30, 2024
@DDtKey
Copy link
Collaborator

DDtKey commented Apr 30, 2024

Hm, if you don’t need to distinguish stderr and stdout, let me clarify, do you need to be able to check that the logs contain something or do you need full access to them?

I guess the common use-case is to verify the content.

Thus, as a first step, we could consider the following suggestion: have something like CmdWaitFor::StdoutOrStderrMessage. This will require a separate structure for the with_cmd_ready_condition() - but it will also allow us to add CmdWaitFor::ExpectedCode there.
Thus, we will be able to do inspect only if it is necessary according to the user's CmdWaitFor.

DDtKey added a commit that referenced this issue May 1, 2024
- support check of exit code
- allow check the message against both `stderr` and `stdout`

Closes #617
@DDtKey
Copy link
Collaborator

DDtKey commented May 1, 2024

I've prepared the implementation of the suggestion

@Warchant please, take a look, let me know what do you think

DDtKey added a commit that referenced this issue May 1, 2024
- support check of exit code
- allow check the message against both `stderr` and `stdout`
- extend `new` to accept `impl IntoIterator<Item = impl Into<String>>`
(e.g to accept `["cmd", "arg1", "arg2"]` as is)

Closes #617
@DDtKey
Copy link
Collaborator

DDtKey commented May 1, 2024

We may still consider your suggestion if you really need full access to the logs, but these approaches don't block each other.

So don't hesitate to reopen if you feel that #622 doesn't cover this issue.

@Warchant
Copy link
Author

Warchant commented May 2, 2024

@DDtKey when I exec something, is there a way to actually read stdout/stderr/both in my test?
Say, container runs a long running process (a server), then I exec some CLI (for example, it generates a keypair). I want to parse that output and use it in my test, check exit code to ensure generation was successful.

@DDtKey
Copy link
Collaborator

DDtKey commented May 2, 2024

@DDtKey when I exec something, is there a way to actually read stdout/stderr/both in my test?
Say, container runs a long running process (a server), then I exec some CLI (for example, it generates a keypair). I want to parse that output and use it in my test, check exit code to ensure generation was successful.

That was the question, if you need access to logs or checking the result is enough.
Now you can check exit-code. It's incorporated into CmdWaitFor and it will wait the command to finish, with the check of exit code.

I will extend API to return logs for such use-case, and they will be accessible independently (both stdout and stderr, but you can distinguish them).

@DDtKey
Copy link
Collaborator

DDtKey commented May 2, 2024

I'll do that during the week, will try to implement asap.
Slightly limited bandwidth the last few days

@DDtKey
Copy link
Collaborator

DDtKey commented May 9, 2024

Sorry for the delay, bandwidth was quite limited for longer period of time.
But the main part is basically done, I need some time to clean up and finalize this.

I'll notify you once it's ready!

@DDtKey DDtKey added this to the 0.17.0 milestone May 9, 2024
@DDtKey DDtKey self-assigned this May 14, 2024
@DDtKey DDtKey reopened this May 14, 2024
@DDtKey
Copy link
Collaborator

DDtKey commented May 14, 2024

Reopening since I'm working on this and it was an irrelevant status for the issue.

Also, changes I've prepared will be useful for #502 as well

Milestone is assigned and relevant

@DDtKey
Copy link
Collaborator

DDtKey commented May 18, 2024

Hi @Warchant and everybody interested in this feature!

I've spent much more time to bring this into testcontainers due to bandwidth and some experiments.
Had to abandon some of the initial "quick" implementations, I tried several options, but finally ended up with interface in #631.
I believe it brings all the necessary functionality to the Exec interface and also matches expectations of #502 and #606

Thank you for patience!

DDtKey added a commit that referenced this issue May 19, 2024
Closes #502

Exposes `stdout` and `stderr` methods, which returns:
- `tokio::io::AsyncBufRead` impl for async container
- `std::io::BufRead` impl for sync container (under `blocking` feature)

Also, small alignment is performed (related to #617):
- rename the `ExecResult` `stdout`/`stderr` methods to match the
container methods
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment