BIND 10 #384: Bindctl doesn't show complicated config structures
BIND 10 Development
do-not-reply at isc.org
Tue Feb 1 01:28:44 UTC 2011
#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 |
-------------------------------------+-------------------------------------
Changes (by jelte):
* owner: jelte => zhanglikun
Comment:
Replying to [comment:9 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.
>
to be sure, i've made it into spaces instead of \t, and I use a constant
for the width now (also increased it a little bit, to 12 charachters, as
most of our commands are 8 or more chars)
>
> 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]
> }}}
Right. At first I was almost tempted to remove this syntax completely, and
go for positional arguments. But i may have found a solution; cmdparse now
removes the whitespace between [] and {}, unless quoted by ". This is a
bit more code than I would have hoped to add after a review, but at least
there are new tests now too :)
(note, I did this last so if you look at the git log this will come up
first)
> 3. I can't undertand the purpose of following code
> {{{
> 561 if cmd.params['identifier'].startswith('['):
> 562 identifier = identifier[:-1]
> }}}
>
Hmm, i have no idea actually, I've removed it.
> 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))
> }}}
>
ohyeah, ok i simply made it so it now prints 'non_exist_config_item not
found' for the above example
>
> '''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.
>
hmm, that would probably not be that hard to change, but what i'm afraid
for is that this would now be easy-to-use, but as the number of options
increase (and for instance, if we have config for every zone), we might
want to default to not-all. Shall we discuss this with the rest?
> 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
> }}}
>
actually, the non-json option makes a nice list with identifier/value
pairs, and the show_json shows the raw output, this is more visible in
places where we have maps in lists etc, like in the resolver
>
> 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?)
>
oh, no idea, changed it to 70 too :)
> 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 += "/"
> }}}
>
ack, removed.
>
> '''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)
> }}}
>
ok, done, thanks
>
> '''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
>
doh, missed a 'not' there. Also added some more words to explain it :)
(it's the value from the specification, i.e. no local change, and nothing
set in the current config for the configmanager, the value was taken from
the specification)
There's probably more corner cases I have not thought of right now, so
please play around again (although I think that at some point we should
call it 'better than it used to be' and create a new task for any issues
that are left :)
--
Ticket URL: <http://bind10.isc.org/ticket/384#comment:11>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list