feat: added CommandStrategy with SuccessfulCommand enum #782
Conversation
✅ Deploy Preview for testcontainers-rust ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Can't this be achieved with exec_after_start with ExitCode wait condition (it waits for exit)? Additionally, this way allows to rely on a container's state
Or we could consider extending existing wait conditions if it's not enough
But generally speaking, at first glance it looks like duplicate functionality
There was a problem hiding this comment.
Now that you mention it and after a second glance, it feels like duplicate functionality. This was probably an oversight on my end by lack of context.
I can test what you've provided and report back findings - that may aid in either resolving or progressing a solution for this issue.
There was a problem hiding this comment.
After investigating, the difference here is that this adds a "command strategy" similar to "shell strategy" (with an emphasis on SuccessfulCommand) here:
https://node.testcontainers.org/features/wait-strategies/#shell-command
To be more orthogonal / consistent with the Node test container, what do you think about this just being ShellStrategy instead? It has the same functionality, if I'm not mistaken:
Whereas WaitFor is similar to: https://github.com/testcontainers/testcontainers-node/blob/b92f6695de10863c6ea90d169f999b79406d162e/packages/testcontainers/src/wait-strategies/wait.ts#L40
Thoughts?
There was a problem hiding this comment.
We don't have to be fully consistent with other languages. You can find a lot of differences between Go, Java and Node, for example (but we try to be aligned when possible)
Also, we should compare exec_after_start with strategies here, not the WaitFor itself.
However we can consider such strategy for sure, more like "alias" or "short/easier way".
But we need to avoid maintaining of duplicative logic/code. Let me explain:
What are we introducing here that is missing in exec_after_start?
If it's about "easier way" - we need to find a way to compose it more correctly
But if there is a difference in functionality - then it should be very clear to users why to use one over the other
There was a problem hiding this comment.
Let's consider the current implementation to understand if it's clear enough
.with_wait_for(WaitFor::SuccessfulCommand(
CommandStrategy::command(
ExecCommand::new(["pg_isready"]).with_cmd_ready_condition(CmdWaitFor::exit_code(0)),
),
))What confuses me a little bit here:
with_cmd_ready_condition(CmdWaitFor::exit_code(0)- expects exit-code = 0SuccessfulCommand- also expects exit-code = 0 (implicitly)- What if I'd change
with_cmd_ready_conditionto expect non-zero code or something else? - Do I really need to specify this two times as a user?
Perhaps, instead I'd suggest to support something like fail_fast/retry for ExecCommand
And add very simple strategy, like Command(ExecCommand)
So it will look like:
.with_wait_for(
WaitFor::command(
ExecCommand::new(["pg_isready"]).with_cmd_ready_condition(CmdWaitFor::exit_code(0))
)
)There was a problem hiding this comment.
Apologies for the delay, been a busy week here with the weather we've been getting in Arkansas. 😅
I've pushed an update that I believe is something that aligns... I was hesitant on the CmdWaitFor::exit_code(0) in WaitFor::command(), but added it anyway.
...however, now that I think about it, we may want to not force that specific exit code, but allow users to specify one on their own. Thoughts?
Nevermind. I've allowed this to be more general than specific.
| #[derive(Debug, Clone)] | ||
| pub struct CommandStrategy { | ||
| expected_code: i64, | ||
| poll_interval: Duration, | ||
| command: ExecCommand, | ||
| fail_fast: bool, | ||
| } |
There was a problem hiding this comment.
It seems unchanged - we still have expected_code here which may contradict to ExecCommand wait conditions
The idea was to change how it's organized and implemented
#782 (comment)
There was a problem hiding this comment.
Apologies for this overlook -- updated!
sutt0n
force-pushed
the
feat/command-strategy
branch
from
e9caf57 to
cee5d7e
Compare
|
Force pushed to fix the commit message... hard to write Rust while holding a sleeping baby. 😅 |
Potentially resolves #702
This introduces a new wait strategy,
CommandStrategy. The goal here is to wait for a successful command (exit code 0) prior to marking a container as ready. This polls the container state to ensure it's running first, and then executes the given command. Once the command has an exit code of 0, then the strategy has completed.There are options to fail fast, as well as customize the exit code, just to expose flexibility for the strategy.
Open to changes / updates, if required -- this language is not my day-to-day.