Opened 7 years ago

Closed 7 years ago

#3028 closed defect (fixed)

Cmdctl print_settings - conflict between behavior and help documentation

Reported by: bconry Owned by: vorner
Priority: medium Milestone: Sprint-20130903
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 2 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

> Cmdctl help
Module Cmdctl   Interface for command and control
Available commands:
    help        Get help for module.
    print_settings
            Print some_string and some_int to stdout
    shutdown    shutdown cmdctl
> Cmdctl print_settings
{
    "accounts_file": "/home/bconry/support/bind/10/bind10-1.1.0/inst/etc/bind10/cmdctl-accounts.csv", 
    "cert_file": "/home/bconry/support/bind/10/bind10-1.1.0/inst/etc/bind10/cmdctl-certfile.pem", 
    "key_file": "/home/bconry/support/bind/10/bind10-1.1.0/inst/etc/bind10/cmdctl-keyfile.pem"
}

I don't know exactly what some_string and some_int are supposed to be, but I do know that JSON doesn't qualify for either.

Subtickets

Change History (14)

comment:1 Changed 7 years ago by vorner

Hmm. It seems like a stray test command. I think that one should be removed completely.

comment:2 Changed 7 years ago by muks

  • Estimated Difficulty changed from 0 to 2

comment:3 Changed 7 years ago by shane

  • Milestone changed from New Tasks to Next-Sprint-Proposed

comment:4 Changed 7 years ago by muks

  • Milestone changed from Next-Sprint-Proposed to Sprint-20130806

Putting to current sprint as we are running out of tickets.

comment:5 Changed 7 years ago by vorner

  • Owner set to vorner
  • Status changed from new to accepted

comment:6 Changed 7 years ago by vorner

  • Owner changed from vorner to UnAssigned
  • Status changed from accepted to reviewing

Ready for review.

The command is apparently testing only. So I removed it from the spec file to hide it from users. I think this is minor enough not to require changelog entry ‒ the command is still there (because tests use it), it just isn't shown in CmdCtl help and isn't tab-completed ‒ so nothing that uses it would break.

comment:7 follow-up: Changed 7 years ago by muks

  • Owner changed from UnAssigned to vorner

Hi Michal

Your patch looks good, but I don't like this command at all. Primarily it seems to be used to test that command_handler() handles commands. We should do one of the following:

  • Make it a very simple test returning some known fixed string and call it a test_command
  • Make it a publicly documented command that prints the settings, and use it in testing as-is
  • Just remove it altogether (other commands like shutdown won't work without being handled in cmdctl). Also remove the commented line in b10-cmdctl.xml.
  • Put it in a derived class MyCommandControl that exists in cmdctl_test.py that calls super() for all other commands except test_command.

But for now if you want to simply merge this, please go ahead.

comment:8 Changed 7 years ago by muks

Oh BTW, we do require ChangeLog for this as this command (however useless it was) was publicly visible before in the bindctl help.

comment:9 in reply to: ↑ 7 ; follow-ups: Changed 7 years ago by vorner

  • Owner changed from vorner to muks

Hello

Replying to muks:

  • Just remove it altogether (other commands like shutdown won't work without being handled in cmdctl). Also remove the commented line in b10-cmdctl.xml.

I did something similar. Now I use the shutdown command for the test.

What comment line do you mean? I can't seem to find one.

  • Put it in a derived class MyCommandControl that exists in cmdctl_test.py that calls super() for all other commands except test_command.

That would make little sense. We would be testing that the derived class works and we wouldn't enter the real code in that test at all.

Proposed changelog:

[func]		vorner
The CmdCtl's command "print_settings" was removed. It served no real purpose
and was just experimental leftover from early development.

comment:10 in reply to: ↑ 9 Changed 7 years ago by jreed

Replying to vorner:

Replying to muks:

Also remove the commented line in b10-cmdctl.xml.

What comment line do you mean? I can't seem to find one.

In src/bin/cmdctl/b10-cmdctl.xml :)

(The docbook source for the manual page documentation.)

comment:11 in reply to: ↑ 9 ; follow-up: Changed 7 years ago by muks

  • Owner changed from muks to vorner

Hi Michal

Replying to vorner:

Hello

Replying to muks:

  • Just remove it altogether (other commands like shutdown won't work without being handled in cmdctl). Also remove the commented line in b10-cmdctl.xml.

I did something similar. Now I use the shutdown command for the test.

Is it checked that shutdown() method is called in this case?

What comment line do you mean? I can't seem to find one.

This line in b10-cmdctl.xml:

<!-- NOTE: print_settings is not documented since I think will be removed -->
  • Put it in a derived class MyCommandControl that exists in cmdctl_test.py that calls super() for all other commands except test_command.

That would make little sense. We would be testing that the derived class works and we wouldn't enter the real code in that test at all.

I meant testing it as a virtual function where command_handler() is not invoked directly, but through some other method. In any case, the approach taken of removing this command entirely is good.

Proposed changelog:

[func]		vorner
The CmdCtl's command "print_settings" was removed. It served no real purpose
and was just experimental leftover from early development.

This is good too.

comment:12 in reply to: ↑ 11 Changed 7 years ago by vorner

  • Owner changed from vorner to muks

Hello

Replying to muks:

Is it checked that shutdown() method is called in this case?

It is now.

What comment line do you mean? I can't seem to find one.

This line in b10-cmdctl.xml:

Thank you, now I found it :-).

comment:13 Changed 7 years ago by muks

  • Owner changed from muks to vorner

Hi Michal

I've pushed a minor fix to the branch. Please review it.

If you are ok with it, you can go ahead and merge this branch.

comment:14 Changed 7 years ago by vorner

  • Resolution set to fixed
  • Status changed from reviewing to closed

Thank you, closed.

Note: See TracTickets for help on using tickets.