Log errors when incorrect commands are used in batch mode#858
Log errors when incorrect commands are used in batch mode#858
Conversation
93f48b0 to
c60cf99
Compare
|
Hey @PawelPlesniak, this is ready for review :) |
|
This is generally very nice. One small change I would make is in the comments. Otherwise nice work! |
| except click.UsageError as e: | ||
| raise DruncBatchShellArgError(e) from e | ||
|
|
||
| parser = ctx.command.make_parser(ctx) |
There was a problem hiding this comment.
This and the following 5 lines should be checkable simply by checking the UnifiedShellMode - if not interactive, this will suffice.
|
on |
|
I will make the suggested changes later today and get this merged, nice work! |
|
I added a small change to update the logic that selects tthe UnifiedShellMode. I think this command needs a little more work - I ran the suggested manual tests and saw the following when running: This implementation is slightly unclear - when looking at the extra arguments, the way that you have filtered for the arguments (by looking at the |

Description
Fixes issue #753
Will give feedback in case there is a bad command when inputting as a drunc batch/semibatch mode.
This change adds in a validation check to ensure that the commands parsed by via the batch mode work.
It does it in a few steps. Note the following example with
unknown_cmd['boot', 'unknown_cmd', 'start-run', '--run-number', '1', 'wait', '20'])If it identifies a command, mark it as a command and move on to the next one
If it doesn't identify a command:
This splits the arguments into a list of command, argument pairs (
[('boot', ['unknown_cmd']), ('start-run', ['--run-number', '1']), ('wait', ['20'])])make_context, go through each command and argument pair and see if its possible to form a contextboot unknown_cmd) is not valid command.This should be robust as it will go through the definitive list of commands as defined from click. It shoulnd't need maintenance as long as people properly tell click of their new commands.
Type of change
List of required branches from other repositories
None
Suggested manual testing checklist
drunc-unified-shell ssh-standalone config/daqsystemtest/example-configs.data.xml local-1x1-config testdrunc-unified-shell ssh-standalone config/daqsystemtest/example-configs.data.xml local-1x1-config test boot wait 5 confdrunc-unified-shell ssh-standalone config/daqsystemtest/example-configs.data.xml local-1x1-config test unknown_cmddrunc-unified-shell ssh-standalone config/daqsystemtest/example-configs.data.xml local-1x1-config test boot sleep 5 confboot sleep. Making this context will fail.)drunc-unified-shell ssh-standalone config/daqsystemtest/example-configs.data.xml local-1x1-config test boot wait 5 conf logs --name ru-controller --user emmuhammdrunc-unified-shell ssh-standalone config/daqsystemtest/example-configs.data.xml local-1x1-config test boot wait 5 conf logs --namEEe ru-controller --user emmuhammdrunc-unified-shell ssh-standalone config/daqsystemtest/example-configs.data.xml local-1x1-config ${USER}-local-test boot wait 5 conf wait 3 start --run-number 142 enable-triggers wait 10 disable-triggers sleep 3 drain-dataflow stop-trigger-sources stop scrap terminateDeveloper checklist
Prior to marking this as "Ready for Review"
Tests ran on: np04-srv-028 from release NFD_DEV_260323_A9
Unit tests - some tests can't be ran on the CI. This is documented. If this PR checks a feature that can't be tested with CI, this has been marked appropriately.
Integration tests - the
daqsystemtest_integtest_bundlerequires a lot of resources, and connections to the EHN1 infrastructure. Check the cross referenced list if you can't run these. The developer needs to run at least the .pytest --marker) passeddaqsystemtest_integtest_bundle.sh -k minimal_system_quick_test.pydaqsystemtest_integtest_bundle.shFinal checklist prior to marking this as "Ready for Review"
Reviewer checklist
druncare in the log filesdruncfailure appears:Once the features are validated and both the unit and integration tests pass, the PRs is ready to be merged.
Choose one of the following an complete all substepsPrior to merging
Once completed, the reviewer can merge the PR.
Notification message for #daq-sw-librarians Slack channel
For an single merge that changes the user workflow
For co-ordinated merge