-
Notifications
You must be signed in to change notification settings - Fork 219
Description
The needs()
function should propagate run-time task arguments (%params
and @args
) from the calling task down to the "needed" tasks
Edits:
As it turns out: this bug is really easy to fix, as it appears to stem from a typo, introduced initially by PR #1157 (the fix for #1066 ) with 48c737b (ultimately merged into master with 7cf0ca4). Some other sharp edges introduced by PR #1157 seem to have already been handled by #1188, but not this one.
I am preparing a PR with accompanying tests.
Describe the bug
The needs()
function has a bug which prevents it from implicitely propagating the calling task's params/args down to the "needed" tasks when it runs them.
How to reproduce it
Steps to reproduce the behavior:
- In a brief
Rexfile
, define two simple tasks (task_a
&task_b
) that dump their run-time arguments (@_
) onSTDERR
- Make sure that
task_b
callsneeds("task_a")
- From the shell, launch
rex task_b
(with appropriate host & auth info) and observe the results
The dump fromtask_a
is empty, whereastask_b
normally displays itsparams
. - Also try
rex task_b --greetings=Hello
, and observe that the result is similar to the previous trial.
Shortest code example that demonstrates the bug:
Rexfile
# Rexfile
### ex: set ft=perl :
use Rex -feature => [qw(1.13 exec_autodie)];
use DDP; # alias for `Data::Printer`, used just for pretty display of dumps.
desc "Display host name";
task hostname => sub {
say STDERR "hostname: \@_ : "; DDP::p(@_);
say STDERR ">>>>>>>>>>>>>>>>>>>>>>>>>>>>>";
say ''. run('hostname');
};
desc "Display host info";
task hostinfo => sub {
say STDERR "hostinfo: \@_ : "; DDP::p(@_);
say STDERR ">>>>>>>>>>>>>>>>>>>>>>>>>>>>>";
needs "hostname";
};
1;
Run the example from the shell
Execute rex
within the directory where the above Rexfile
resides (with appropriate host and authentication switches).
$ rex hostinfo --greetings=Hello
[2021-09-26 18:26:45] INFO - Running task hostinfo on medusa
hostinfo: @_ :
[
[0] {
greetings => "Hello",
hostinfo => 1
},
[1] []
]
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
hostname: @_ :
[]
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
medusa
Expected behavior
The needs()
function should implicitly propagate the calling task's params/args down to the "needed" tasks when it runs them, as documented and seemingly intended by the current code base (see notes below).
Circumstances
- Rex version: (R)?ex 1.13.4 (also on latest branch master)
- Perl version: v5.34.0
- OS running rex: ~~~ (irrelevant)
- OS managed by rex: ~~~ (irrelevant)
- How rex was installed: git clone repo
Debug log
Notes
It turns out: this is really easy to fix, as it appears to stem from a typo.
The culprit
Here's the offending portion of code in Rex::Commands
.
# lib/Rex/Commands.pm
...
sub needs {
...
if ( @args && grep ( /^\Q$task_name\E$/, @args ) ) {
Rex::Logger::debug( "Calling " . $task_o->name );
$task_o->run( "<func>", params => \@task_args, args => \%task_opts );
}
elsif ( !@args ) {
Rex::Logger::debug( "Calling " . $task_o->name );
$task_o->run( "<func>", params => \@task_args, args => \%task_opts );
}
...
}
Notice how params
and args
are mixed up (interchanged) when being passed to run
!! This certainly looks like a typo.
The current test suite does not catch the issue. The tests in needs.t
do not cover propagation of parameters/args.
Quick fix
For a fix with minimal changes, just patch the two occurrences of the $task_o->run
call, within the body of the needs()
subroutine (in Rex::Commands
), as below:
- $task_o->run( "<func>", params => \@task_args, args => \%task_opts );
+ $task_o->run( "<func>", params => \%task_opts, args => \@task_args );
Alternative fix (slightly better, imho)
There does not appear to be any particular reason for having two identical invocations of the ->run
method in the above code snippet.
So, the offending code can be replaced by the following -logically equivalent- snippet. I will put that in a separate commit; in case there are other (historical?) reasons to keep the two branches of the if statement around.
In any case, all tests pass either way.
# lib/Rex/Commands.pm
...
sub needs {
...
# edit: further simplified the logic.
if ( !@args || grep ( /^\Q$task_name\E$/, @args ) ) {
Rex::Logger::debug( "Calling " . $task_o->name );
$task_o->run( "<func>", params => \%task_opts, args => \@task_args );
}
...
}
Edit: Digging in repo history, it seems that the if ... elsif ...
form existed since the very initial introduction of needs()
by commit 95d3e91, but even at that time (and probably ever since), those two arms of the if
statement always did exactly the same thing... So I can't think of any valid reason to keep them around.
Activity
Add TODO tests for issue RexOps#1508 which check and describe how nee…
Add TODO tests for issue RexOps#1508 which check and describe how nee…
Fix RexOps#1508. Related tests are now passing
Remove TODO markers from related tests following the fix for RexOps#1508
Fix RexOps#1508 w/ some code cleanup. All tests still pass.
Add "TODO" tests related to issue RexOps#1508
Fix RexOps#1508 simply by correcting typo on a couple lines
Fix RexOps#1508 by correcting a typo + minimal code cleanup
Update ChangeLog, remove "TODO" mark from tests related to RexOps#1508
needs()
function should propagate calling task's params/args down to needed tasks #1509Tidy tests related to RexOps#1508 (to satisfy xt/author/perltidy.t)
tabulon commentedon Sep 27, 2021
Hi @ferki,
PR (#1509) which fixes this issue (#1508) is ready, but maintainer approval is required to launch CI workflow (for first time contributors to the project, as is the case here).
Otherwise, all tests pass locally with
dzil test --all
.Should I just mark th PR as "Ready for Review" so as to get out of the chicken-and-egg situation ?
[-]The `needs()` function does not propogate run-time task arguments (`%params` and `@args`) from the calling task down to the "needed" tasks[/-][+]The `needs()` function should propogate run-time task arguments (`%params` and `@args`) from the calling task down to the "needed" tasks[/+]ferki commentedon Oct 25, 2021
Thanks for the report, @tabulon! Kudos for the amazing amount of details 👍
Yeah, unfortunately that commit had somehow caused quite some trouble. Hopefully the affected behavior is nicely covered by tests after this one.
Hmm, I don't think parameter and argument passing is explicitly documented for
needs
(yet). The docs only mention "same server configuration as the calling task" which refers to "execute the needed task on the same connection that is currently active". Though I can confirm that the previous behavior has changed (seemingly inadvertently) on the aforementioned commit, so it's a bug to fix.Yes, I agree that it seems to be a typo, and this aspect of the behavior is untested. Well spotted!
That's an amazing depth of analysis, thanks for going the extra mile by looking at the history! ❤️
I also don't think there's any reason to keep duplicate code. As an aside, I wonder if there's a good way to detect similar duplications? 🤔 Perhaps that's something for the scope of Perl::Critic::TooMuchCode.
I'll also re-read the history again, but I think we should be fine as long as we:
ferki commentedon Oct 25, 2021
I've modified the repo settings to be more relaxed about triggering workflows.