BIND 10 #384: Bindctl doesn't show complicated config structures

[BIND 10 Development]

#384: Bindctl doesn't show complicated config structures
-------------------------------------+-------------------------------------
                 Reporter:  vorner   |                Owner:  zhanglikun
                     Type:  defect   |               Status:  reviewing
                 Priority:  major    |            Milestone:  R-Team-
                Component:  bind-    |  Sprint-20110208
  ctl                                |           Resolution:
                 Keywords:           |            Sensitive:  0
Estimated Number of Hours:  5.0      |  Add Hours to Ticket:  0
                Billable?:  1        |          Total Hours:  0
                Internal?:  0        |
-------------------------------------+-------------------------------------

Comment (by zhanglikun):

 Hi Jelte, the following is my review result, except the commented part,
 others looks ok.


 '''bindctl/bindcmd.py'''

 1. do_help() in bindctl/bindcmd.py (and module_help() in moduleinfo.py)
 {{{
         if len(n) >= 8
 }}}

 length 8 is hardcoded, do we need to check the width of '\t' ?, since it
 may has different value on different system.


 2. When I do the test, I find the following command will make bindctl to
 crash.
   "config set identifier= Auth/datasources value=[1,2] ", it's caused by
 the following line code
 {{{
 params[param_count - 2] += params[param_count - 1] #line 335 in bindcmd.py
 }}}

   also, I try to use bindctl like a crazy user by inputing command "config
 set identifier= [1] value='[1,2]'", it make the bindctl crash. It is
 caused by the following line code
 {{{
    module_name = identifier.split('/')[1]
 }}}
 3. I can't undertand the purpose of following code
 {{{
 561                     if cmd.params['identifier'].startswith('['):
 562                         identifier = identifier[:-1]
 }}}

 4. when I run the command  "config show Auth/non_exist_config_item", seems
 the error message isn't meaningful now, compared with the old error
 message "Error: Auth/non_exist_config_item not found"
 {{{
 646         except isc.cc.data.DataNotFoundError as dnfe:
 647             print("Error: " + str(dnfe))
 }}}


 '''bindctl/bindctl-source.py.in'''
 1. there is one new added parameter for command "config show", I am not
 sure if we really need it, I prefer to always show all child elements for
 specified identifier.
 when user doesn't specify the identifier for command "config show", just
 show all elements recursively for all modules.

 2. If I understand correctly, the difference between 'show' and
 'show_json' is: the latter one just print the value without type and
 default value, is it right?
 If that's true, I personly opinion is: merge them to one command by adding
 another parameter to command.
 {{{
 config show Auth/database_file  value_only=True
 }}}

 it will work like
 {{{
 config show_json Auth/database_file
 }}}

 '''bindctl/moduleinfo.py'''

 1. I find the width is set to '50'(line 171 and 183 in moduleinfo.py),
 which is different with '70' set in line 243, is it on purpose?)

 2. "len(value_map) > 1" is not needed in line 593
 {{{
 593                     elif len(value_map) > 1 and value_map['type'] ==
 'list' \
 594                          and (value_map['value'] != []):
 595                         # do not print content of non-empty lists if
 596                         # we have more data to show
 597                         line += "/"
 }}}


 '''config/ccsession.py'''
 1.
 {{{
 402         if value not in cur_list:
 403             cur_list.append(value)
 404
 405         self.set_value(identifier, cur_list)
 }}}

 Above code can be refactored(If the value already exist, we don't need to
 set the same value again) as:

 {{{
 402         if value not in cur_list:
 403             cur_list.append(value)
 404             self.set_value(identifier, cur_list)
 }}}


 '''config/config_data.py'''
 1. in the function description of get_value_maps(): "default: true if the
 value has been changed"
 I am not I have understood what 'default' means here, is it mean the
 current value has been changed and it's different with default value? if I
 understand correctly, could you use another word to discribe it? like
 same_with_default

-- 
Ticket URL: <http://bind10.isc.org/ticket/384#comment:9>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development