[PFCWD] Fix 'start' pfcwd command

[shlomibitton]

- What I did
The 'config' method is getting arguments from the user CLI and build a proper command for the pfcwd script in a wrong way, causing this issue.

- How I did it
Fix the command structure to align with pfcwd.main script.

- How to verify it
Run "config pfcwd start --action drop ports all detection-time 400 --restoration-time 400"

- Previous command output (if the output of a command-line utility has changed)
config pfcwd start --action drop ports all detection-time 400 --restoration-time 400
Failed to run command, invalid options:
ports
detection-time

- New command output (if the output of a command-line utility has changed)
Command succeeded.

[liat-grozovik]

retest this please

[abdosi]

retest this please

[smaheshm]

"ports" and "detection-time" have been added as click arguments. This code hasn't changed.

@click.option('--action', '-a', type=click.Choice(['drop', 'forward', 'alert']))
@click.option('--restoration-time', '-r', type=click.IntRange(100, 60000))
@click.argument('ports', nargs=-1)
@click.argument('detection-time', type=click.IntRange(100, 5000))

The right syntax would be:

config pfcwd start --action drop --restoration-time 400 all 400

Looks like the documentation shows the same options that you used. Either the documentation should be changed, or the "port", "detection-time" should be changed to click options.

[smaheshm]

you will have to update config/main.py

[shlomibitton]

@smaheshm can you please review the change?
I have simply modified the wrapper from config.main to align with the desired command format on pfcwd.main.
Looks like on the unit-tests we are calling these function directly from pfcwd.main script and not from the config.main script so the test didn't catch this bug.

[smaheshm]

@smaheshm can you please review the change?
I have simply modified the wrapper from config.main to align with the desired command format on pfcwd.main.
Looks like on the unit-tests we are calling these function directly from pfcwd.main script and not from the config.main script so the test didn't catch this bug.

That's right. Not sure why pfcwd wasn't added as a module. Can you add a unit test that uses 'config pfcwd' CLI.

[shlomibitton]

@smaheshm can you please review the change?
I have simply modified the wrapper from config.main to align with the desired command format on pfcwd.main.
Looks like on the unit-tests we are calling these function directly from pfcwd.main script and not from the config.main script so the test didn't catch this bug.

That's right. Not sure why pfcwd wasn't added as a module. Can you add a unit test that uses 'config pfcwd' CLI.

@smaheshm calling the 'config' script to start pfcwd is failing on the unit-test environment, need to further investigate it.
I suggest to merge this PR first to fix this issue and I will handle the 'config' test adaptation on a different PR.
what do you think?

[shlomibitton]

retest this please

[liat-grozovik]

@smaheshm i agree that testing is always good to have. but in this case it is not the scopt. we are missing as a community various of test infra to add tests for bugs fixes.
as this bug is critical as one cannot configure specific pfc wd as before i suggest first to merge the fix and add to the next release backlog the infra for pfc wd testing on vs.

[smaheshm]