Page MenuHomePhabricator

[tbs.cli] Multiple bugs when running external commands
Open, In Progress, Needs TriagePublic3 Estimated Story Points

Description

Running external subcommands works okay in some cases, but not others:

  • Subcommands with or without args work ok
  • Passing flags to subcommands fails. Verbose output shows that _run_external_command() isn't even invoked in such cases
  • However, providing the -h flag does invoke the _run_external_command(), passing --help to it, which causes a crash if the subcommand doesn't accept this flag.
  • Failing to provide required args to a subcommand displays the error output of the subcommand, but also crashes due to returning a non-zero exit code

This task is about resolving the above issues (and potentially others), ensuring that external subcommands run smoothly in all scenarios, including graceful error handling. Adding some integration tests could be a good idea, too.

Acceptance criteria:

  • The cli does not crash on external commands that exit with a non-zero exit code
  • Options with and without args are correctly passed through to external subcommands
  • No flags (including --help) are automatically passed through to external subcommands
  • There are actual test scripts instead of the empty files in the tests/fixtures subdirs currently used for testing add_discovered_subcommands()
  • There are tests that guard against reintroducing the above bugs

Event Timeline

Can you split this task and create one for each of the issues?
That would help both splitting the work and tracking the issues themselves.

I can help with that if you prefer but might bother you asking for details for each xd

Can you split this task and create one for each of the issues?
That would help both splitting the work and tracking the issues themselves.

I'm not sure yet whether that would be actually helpful – I suspect most changes would be to _run_external_command(), which is ~10 lines. Having several people working on such as small area of code at the same time could be counterproductive. I may of course be wrong about this, so I'll tinker with it a little first, and if it makes sense, split it up into subtasks as you suggest.

Can you split this task and create one for each of the issues?
That would help both splitting the work and tracking the issues themselves.

I'm not sure yet whether that would be actually helpful – I suspect most changes would be to _run_external_command(), which is ~10 lines. Having several people working on such as small area of code at the same time could be counterproductive. I may of course be wrong about this, so I'll tinker with it a little first, and if it makes sense, split it up into subtasks as you suggest.

👍

Would it be okay to drop the addition of the --help option to all discovered subcommands, and just have the click command be a pass-through for all and any args and options? Or are we sure that all external commands will accept this flag, as opposed to using another of the commonly used ways to display help?:

  • running the command with no options or args
  • help
  • -help
Slst2020 changed the task status from Open to In Progress.Wed, Nov 23, 1:15 PM

and just have the click command be a pass-through for all and any args and options?

Yep, that'd be ok, iirc the pass-through of the --help command was because click handles --help specially, but the goal was just to pass it through (--help, or whatever else).

and just have the click command be a pass-through for all and any args and options?

Yep, that'd be ok, iirc the pass-through of the --help command was because click handles --help specially, but the goal was just to pass it through (--help, or whatever else).

Nice, that makes things a bit simpler!

I think I've managed to find fixes to the various bugs – will break them up into individual subtasks shortly.

Slst2020 set the point value for this task to 2.Wed, Nov 23, 4:43 PM
Slst2020 changed the point value for this task from 2 to 3.
Slst2020 changed the task status from In Progress to Open.Fri, Nov 25, 2:21 PM
Slst2020 changed the task status from Open to In Progress.Tue, Nov 29, 1:31 PM

Is this resolved now then?
Are you waiting to deploy it on toolsbeta? (that'd be nice)

Is this resolved now then?

The bugs are resolved and merged, but I want to add some tests as well before closing this task.

Are you waiting to deploy it on toolsbeta? (that'd be nice)

Yes – let's do a deploy! Maybe at the end of this iteration?

Yes – let's do a deploy! Maybe at the end of this iteration?

I would say as soon as you want :) (probably the end of this iteration being the latest)