BIND 10 #2967: zonemgr should use general datasource configuration, not Auth/database_file

[BIND 10 Development]

#2967: zonemgr should use general datasource configuration, not Auth/database_file
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  defect        |  vorner
            Priority:  medium        |                       Status:
           Component:  xfrin         |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130709
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  4             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by pselkirk):

 * owner:  pselkirk => vorner


Comment:

 Replying to [comment:15 vorner]:
 > I know it was recommended, but I believe cherry-pick is reasonable for
 one or two commits, not for half of the whole branch. The cherry-picked
 commits are duplicates in history, which is not nice in case the history
 needs to be examined. Would it be OK to rebase the branch on top of master
 once the #2854 is merged (it should be any time soon)?

 Done, now that #2854 is merged.

 > What is the reason for these things?
 >
 > {{{#!python
 >         self.zone_refresh._get_zone_soa = old_get_zone_soa
 > }}}
 >
 > This seems unnecessary, since new `zone_refresh` should be created for
 each test, so it is not needed to restore it. If a new one is not created,
 the tests could influence each other, which is bad.

 This was based on the behavior of the old tests, but they were modifying
 the external `sqlite_ds`, so not really comparable. Anyway, fixed now.

 > It feels very hackish to overwrite the rdata_net global variable. As it
 is sitting at the top of the file, the impression it does is it is a
 constant. We do these things to other modules from tests, to inject mock
 ups, but it seems it should be possible to do it in a cleaner way if it is
 the same module. Also, if one of the asserts fails, the value is not
 restored, which will break other tests.

 The idea was to provide a lightweight way of changing the rdata provided
 by the `find()` method. I changed it to override the `find()` method
 itself, which still feels a bit hackish, but works and has precedent in
 other test code.

 > {{{#!python
 >     def tearDown(self):
 >         pass
 > }}}
 >
 > This is unnecessary. The tearDown method is optional, there's no need to
 have an empty one there.

 Done.

 > When folding the long lines, it might be useful to know that the
 backslash is not needed if it is inside of parentheses

 Okay, thanks.

 > Also, python does not need parentheses around conditionals and when seen
 in the code, pythonists wonder what the catch there is:
 >
 > {{{#!python
 >             if (zone_soa == None):
 > }}}

 Okay, that one was mine, but similar tests are all over the original code.
 I fixed all the one-liners.

 > Looking at this code, strong deja-vu feeling hangs in the air. I'd say
 I've seen such code somewhere (maybe some kind of isc.utils library).
 > {{{#!python
 >             def format_zone_str(zone_name, zone_class):
 > }}}

 At a glance, isc.util an address formatter, but not a zone name formatter.
 I could create one, but I'm not overly concerned with one-line functions.

 > I'd say a zone without SOA might be legal (for example empty), but it
 still is suspicious situation. Should this be a warning instead of info?
 > {{{#!python
 >             if result != ZoneFinder.SUCCESS:
 >                 logger.info(ZONEMGR_NO_SOA, format_zone_str(zone_name,
 zone_class))
 >                 return None
 > }}}

 This seems reasonable, and in line with `zonemgr_add_zone()`.

 This code is borrowed from xfrin, where it returns `XFRIN_ZONE_NO_SOA`,
 but there may be a reason why info would be a more appropriate level.

 > I don't think it is in the scope of the ticket, but we may want to be
 able to specify the datastore name for a configured zone too. This does
 not seem to support that, maybe some kind of comment would be in place?:
 > {{{#!python
 >         clist =
 self._datasrc_clients_mgr.get_client_list(zone_name_class[1])
 > }}}

 I'm not entirely sure what you mean here.

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