[CI] new command implementation for tests #4056
Conversation
|
@AkihiroSuda I will deal with the rebasing once we get all the dependent PR merged-in, but this is ready for review for all intent and purposes. I am not sure we can break this down further in individual PRs (I did break it in individual commits though, that should help review), but LMK if you want me to yank something specific out in a separate PR. |
apostasie
force-pushed
the
ci-tigron-cmd
branch
from
e47f344 to
961d090
Compare
ReviewersAll commits except one are mundane. The one that matters (1892 loc) is: 2bc62a5#diff-66fe4e3b3287c94bcd9942c86d92bb7259988cbc2aa6345a47dc2b56d14f411b - which contains the new command implementation alongside with its tests, examples, etc. One thing we might want to discuss is the change in debugging output (hopefully for increased readability) Visually it will change this: 
Into: 
I am not particularly attached to the actual output (as long as it is more readable), so, if you would prefer it organize / formatted differently, lmk. |
apostasie
force-pushed
the
ci-tigron-cmd
branch
2 times, most recently
from
329b0ca to
c62aea8
Compare
|
FIXED
|
Signed-off-by: apostasie <[email protected]>
Signed-off-by: apostasie <[email protected]>
Signed-off-by: apostasie <[email protected]>
- use internal/com instead of icmd - move pty from test/internal to internal - update go mod, sum, and depguard Signed-off-by: apostasie <[email protected]>
Signed-off-by: apostasie <[email protected]>
- Background() signature change - command creation change Signed-off-by: apostasie <[email protected]>
- Background - Feed, WithFeeder, WithPseudoTTY - use exit code consts where appropriate - some --quiet Signed-off-by: apostasie <[email protected]>
Note this is a large PR, though the bulk of the changes are under a couple of files in
mod/tigron/internal, and probably half of it is just tests.This PR is a tigron changeset addressing a few important issues:
In a shell: icmd does not provide value for us. Most of its features we do not use, and the simple wrapping around golang command exec is causing more issues than it solves - icmd does replace the process standard streams with its own buffers, which gets in the way of using ptys, does not provide proper timeout logic for stuck processes, and does not seem to kill process groups properly.
This PR does away with it, and replace it with an internal com.Command that does offer:
... along with absorbing a large part of the logic that was previously baked into test/command.go (env filtering, cloning, command wrapping).
While this should technically be (mostly) transparent for tests, some of the logic changes with timeout and pty / stdin handling required adaptation of some functions signatures.
Specifically:
Background(timeout)is now simplyBackground(), as the newWithTimeouthelper now applies to any command, and not just the background-ed onesWithPTYno longer supports the extra "writers" argument. To write to stdin, the same methods can now be used whether or not we are working with a PTYWithStdinmethod was ill-named/ill-advised, as it is not setting stdin, but writing to stdin, can be used repeatedly, and lacked flexibility. It has been replaced byFeed(io.Reader)for the simpler cases, andWithFeeder(func()io.Reader)that allows for more complex logic before writing to stdin (likePty(writers)used to)Note that we might want to tweak the current default timeout for commands execution (3 minutes rn).
Blocked by:
Preferable to merge after:
[MAJOR] content digest not found) #4062TODO: