Opened 9 years ago

Closed 9 years ago

#384 closed defect (fixed)

Bindctl doesn't show complicated config structures

Reported by: vorner Owned by: zhanglikun
Priority: medium Milestone: R-Team-Sprint-20110222
Component: ~bind-ctl (obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 5.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

It is impossible to show items inside complicated configuration structures. For example, if there's list of maps, there's no way to show what is inside the map:

> config show Recurse/listen_on/
address/	map	
> config show Recurse/listen_on/address/
Error: /Recurse/listen_on/address/ not found
> config show Recurse/listen_on[0]
Error: /Recurse/listen_on[0] not found
> config show Recurse/listen_on/0
Error: /Recurse/listen_on/0 not found

It should be possible to address even items inside a list.

Subtickets

Change History (21)

comment:1 Changed 9 years ago by jelte

Right.

In some ways, this is simply incomplete; there is no way to address items in a list.
In other ways, bindctl is 'too smart'; it tries (badly) to make pretty results, thereby hiding essential data.
The first problem also makes it harder (or impossible) to get for instance default values.

I also found another problem in the parser, in that it sometimes erroneously sticks a '/' at the end of a key in a map.

For the first item, I propose to add this feature;

  • For list elements, individual items can be addressed with [i], where i is the element number.
  • I think we then also want some form of getIdentifier(); given a piece of data, and some context, return the full identifier to that piece of data.

There is an optional 'advanced' thing i'm half thinking of, since lists will probably often contain map elements, perhaps we also want some form of 'primary key' in these maps, that we can use directly in addressing. However, this is a. perhaps not really intuitive, and b. would mean that that key be a string, and unique. (for instance, if you have a list of zones somewhere, where 'zone' contains other info, you'd be able to use an identifier like 'my_module/zones/example.com/masters', where 'example.com' is obviously not a built-in name but an element of the map in the list my_module/zones).

For the second item; I propose I remove the 'smartness' and reduce it to two options; either print only 1 'level' of config/data/whatever, or print it all completely in JSON format (so you can copy paste back into input if you want).

comment:2 Changed 9 years ago by jelte

  • Owner set to jelte
  • Status changed from new to assigned

pulled out the /-thing and the list addressing (tickets #403 and #405, respectively), so we can focus on the bindctl output in this one.

comment:3 Changed 9 years ago by jreed

Running test: config_data_test.py
....................F...
======================================================================
FAIL: test_get_value_maps (__main__.TestMultiConfigData)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/backup/isc-work-svn/svn/branches/trac384/src/lib/python/isc/config/tests/config_data_test.py", line 431, in test_get_value_maps
    {'default': True, 'type': 'map', 'name': 'Spec2/item6', 'value': {}, 'modified': False}], maps)
AssertionError: Lists differ: [{'default': False, 'type': 'i... != [{'default': False, 'type': 'i...

First differing element 5:
{'default': True, 'type': 'map', 'name': 'Spec2/item6', 'value': {}, 'modified': False}
{'default': True, 'type': 'string', 'name': 'Spec2/item6/value1', 'value': 'default', 'modified': False}

Second list contains 1 additional elements.
First extra element 6:
{'default': False, 'type': 'integer', 'name': 'Spec2/item6/value2', 'value': None, 'modified': False}

  [{'default': False,
    'modified': False,
    'name': 'Spec2/item1',
    'type': 'integer',
    'value': 2},
   {'default': True,
    'modified': False,
    'name': 'Spec2/item2',
    'type': 'real',
    'value': 1.1},
   {'default': False,
    'modified': True,
    'name': 'Spec2/item3',
    'type': 'boolean',
    'value': False},
   {'default': True,
    'modified': False,
    'name': 'Spec2/item4',
    'type': 'string',
    'value': 'test'},
   {'default': True,
    'modified': False,
    'name': 'Spec2/item5',
    'type': 'list',
    'value': ['a', 'b']},
   {'default': True,
    'modified': False,
-   'name': 'Spec2/item6',
+   'name': 'Spec2/item6/value1',
?                       +++++++

-   'type': 'map',
+   'type': 'string',
+   'value': 'default'},
+  {'default': False,
+   'modified': False,
+   'name': 'Spec2/item6/value2',
+   'type': 'integer',
-   'value': {}}]
?            ^^

+   'value': None}]
?            ^^^^


----------------------------------------------------------------------
Ran 24 tests in 0.034s

FAILED (failures=1)
*** Error code 1

Stop.
make: stopped in /backup/isc-work-svn/svn/branches/trac384/src/lib/python/isc/config/tests

comment:4 Changed 9 years ago by jreed

I tested this some. See my examples:

> config show Recurse
Recurse/timeout 2000    integer (default)
Recurse/retries 0       integer (default)
Recurse/forward_addresses       []      list    (default)
Recurse/listen_on/      list    (default)

> config show Recurse/listen_on/
Recurse/listen_on/[0]/address   null    string  
Recurse/listen_on/[0]/port      null    integer 
Recurse/listen_on/[1]/address   null    string  
Recurse/listen_on/[1]/port      null    integer 

> config show Recurse/forward_addresses
Recurse/forward_addresses       []      list    (default)

> config show all Recurse
Recurse/timeout 2000    integer (default)
Recurse/retries 0       integer (default)
Recurse/forward_addresses       []      list    (default)
Recurse/listen_on[0]/address    "::1"   string  (default)
Recurse/listen_on[0]/port       5300    integer (default)
Recurse/listen_on[1]/address    "::1"   string  (default)
Recurse/listen_on[1]/port       5300    integer (default)

> config add Recurse/forward_addresses {} 

> config show Recurse/forward_addresses
Recurse/forward_addresses[0]/   map     (modified)

> config set Recurse/forward_addresses[0]/address "1.2.3.4"

> config show Recurse/forward_addresses
Recurse/forward_addresses[0]/address    "1.2.3.4"       string  (modified)
Recurse/forward_addresses[0]/port       53      integer (default)

> config diff
{'Recurse': {'forward_addresses': [{'address': '1.2.3.4'}]}}

> config show_json Recurse
{"forward_addresses": [{"address": "1.2.3.4"}]}

comment:5 Changed 9 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing

Some noteworthy changes in recent commits;

  • now gets the correct default values (see for instance the two default listen_on values)
  • improved help data for config commands
  • improved printing/indentation of help data in general
  • 'go' command now accepts absolute addresses correctly, (i.e. starting with '/'), it checks if you 'go' to a location that does not exist (i.e. not in specfile), and it supports the use of '..' (to go up one level)
  • for config show, it will also show map elements (we're never really interested in just the map name)

I think that's about enough for this for now, could you please take a look?

(for code review: i have synced with trunk a couple of times, so to see the actual differences it's probably easiest to diff trunk and this branch at rev 4152)

comment:6 Changed 9 years ago by stephen

  • Milestone set to R-Team-Sprint-20110125

comment:7 Changed 9 years ago by stephen

  • Estimated Difficulty changed from 0.0 to 5.0

comment:8 Changed 9 years ago by zhanglikun

  • Owner changed from UnAssigned to zhanglikun

comment:9 follow-up: Changed 9 years ago 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.


  1. 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]
  1. I can't undertand the purpose of following code
    561                     if cmd.params['identifier'].startswith('['):
    562                         identifier = identifier[:-1]  
    
  1. 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.

  1. 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?)
  1. "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

comment:10 Changed 9 years ago by zhanglikun

  • Owner changed from zhanglikun to jelte

comment:11 in reply to: ↑ 9 Changed 9 years ago by jelte

  • Owner changed from jelte to zhanglikun

Replying to 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)


  1. 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)

  1. 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.

  1. 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?

  1. 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 :)

  1. "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 :)

comment:12 Changed 9 years ago by jelte

Oh, by the way, I also noted another problem, and fixed that, see the description of commit ef67acec41e9b83d4aacff8de12e6a3e37628227

comment:13 Changed 9 years ago by zhanglikun

Replying to jelte:

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?

OK, make sense, let's keep it.

When I doing the test, I find some problem.

the following command has error.

config go Resolver/

but it works with the following command

config go /Resolver/

and also I find after "config go Resolver/", there is no way to config go to sub config items,
eg. I can't "config go forward_address".

When I check the code, I find my above review comments had error, the following codes should not be removed. Sorry for that. It will be used after command "config go /Resolver/foward_address", when running the following command.

Resolver/foward_address>config set [0]/address "1.1.1.1"
561                     if cmd.params['identifier'].startswith('['):
562                         identifier = identifier[:-1]  

so commit "086b420bfba5b31f13e2828ca1be842a8faad4aa" should be reverted.

All others looks nice.

comment:14 Changed 9 years ago by zhanglikun

  • Owner changed from zhanglikun to jelte

comment:15 Changed 9 years ago by jelte

  • Owner changed from jelte to zhanglikun

Oh, yeah, my 'fix' for that error also broke the 'config go' command as you have found...

fixed and reverted in cad6c0a62b3c5cace703d9863d64dbdb1e047046 and 6bbb42b7fa668d7a8fcc3cf3809365179705a3a2

Jeremy also had a few comments on output, which i addressed while i was here too, please see commit b6bce27baf454101b3755d46877ac84c5eef20f2

(it checks for interactive terminal and does not set the prompt if there isn't, ie. no '>' and locations if you pipe a script to it, and it changes the output on errors a bit)

comment:16 Changed 9 years ago by zhanglikun

yes, it works now, go ahead to merge. I will keep silent for this ticket now, :)
though I find another problem, I will make one new ticket for it. :).

comment:17 Changed 9 years ago by zhanglikun

  • Owner changed from zhanglikun to jelte

comment:18 Changed 9 years ago by zhanglikun

one quick review suggestion:
could we remove the line 390 (raise ke) in bindcmd.py?, so that it doesn't crash when inputting the following command:

>config go Resolver/forward[0][1]
>config set address "1.11.1.1"

comment:19 Changed 9 years ago by jelte

  • Owner changed from jelte to zhanglikun

I've removed the raise, but this also showed a more fundamental problem (that go command should have failed in the first place), which i think i also fixed now, but please try and run your set of commands again :)

comment:20 Changed 9 years ago by zhanglikun

just tested , works well. go ahead

comment:21 Changed 9 years ago by jelte

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

Thanks :)

Merged, closing ticket.

Note: See TracTickets for help on using tickets.