fix(core): add forwardallargs option to run-commands builder

[techgerm]

• add a forwardAllArgs option to run-commands builder that defaults to true when using command and
  false when using commands
• My first time contributing a PR to this repo ... I read through all contributing documentation but please let me know if I missed something.

ISSUES CLOSED: #3335

Current Behavior

• All arguments passed down to the run-commands builder are always forwarded to all commands which can break user commands

Expected Behavior

• Users should have an option to control this behavior and can now do so with the fowardAllArgs option

Related Issue(s)

Fixes #
Closes #3335

[techgerm]

I could really use a bit of help in debugging the one failing unit test.
The git-hasher "should work" unit test appears to be failing on line 70 after running run('git add .') but I've got no clue as to why this would be failing or how my changes affected this test.
To add to the confusion, run('git add .') on line 35 just above seems to be working just fine 😕

[bekos]

hi @germanp173, thx a lot for the PR 👍 reading the discussion with @vsavkin on the issue, i have a following comments:

• single command should keep behaving as before, but I don't think it's a problem supporting a forwardAllArgs also as you did
• multiple commands should support the forwardAllArgs parameter per command, as a global parameter would have the same problems, ie some commands want to pass all and other's not. So the supported syntax should be like this:

commands: [
  {
    command: 'command one',
    forwardAllArgs: true
  },
  {
    command: 'command two',
    forwardAllArgs: false
  },
 ...
]

• I believe that forwardAllArgs should default to true in all cases for now, otherwise it will be a breaking change 🤔

WDYT? Tell me if there is anything that needs clarification.

[techgerm]

hey @bekos, thanks for taking a look at my PR!
I agree with what you've proposed and have updated my branch. Let me know if I missed something.

[bekos]

hi @germanp173 thanks for your changes 💪 sorry for that, but one thing that I didn't realize on my previous comment, is what will happen if someone by mistake uses the commands (plural) with the forwardAllArgs like this:

commands: [...],
forwardAllArgs: false,

Atm, we don't throw any error and the forwardAllArgs is actually ignored, and this will be confusing 😞 IMO we have the option to either throw an error if commands and forwardAllArgs is used together, or totally drop the forwardAllArgs in command (which would probably make more sense, as it is derived from it's actual usage) and keep the forwardAllArgs inside each of the commands. This will also decrease the API/schema surface which is generally a good thing 😃 ie command is a shortcut for commands: [ { command, forwardAllArgs: true }], and if any adjustment is needed anyone can use directly the commands. WDYT?

[techgerm]

ah good call, yeah makes sense and it does make it easier to follow from a user perspective 👌

[bekos]

@germanp173 Thanks for your contribution 😃 Awesome job 🎉

[techgerm]

@bekos thanks!

[cliffmeyers]

I would love to see this backported to 9.x. As far as I can tell it's not supported in 9.7.0:

An unhandled exception occurred: Schema validation failed with the following errors:
  Data path ".commands[0]" should NOT have additional properties(forwardAllArgs).

[github-actions]

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.