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        |  pselkirk
            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 vorner):

 * owner:  vorner => pselkirk


Comment:

 Hello

 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)?

 And, now the usual bunch of nitpicking:

 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.

 {{{#!python
         self.assertRaises(ZonemgrException,
 self.zone_refresh.zone_refresh_success, ("example.test.", "CH"))
         self.assertRaises(ZonemgrException,
 self.zone_refresh.zone_refresh_success, ZONE_NAME_CLASS3_IN)
         rdata_net = old_rdata_net
 }}}

 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.

 {{{#!python
     def tearDown(self):
         pass
 }}}

 This is unnecessary. The tearDown method is optional, there's no need to
 have an empty one there.

 When folding the long lines, it might be useful to know that the backslash
 is not needed if it is inside of parentheses, like here (but there are
 many other cases as well):

 {{{#!python
         self._set_zone_next_refresh_time(zone_name_class,
                                          self._get_current_time() + \
                                              self._random_jitter(max,
 jitter))
 }}}

 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):
 }}}

 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):
                 """Helper function to format a zone name and class as a
 string
                 of the form '<name>/<class>'.
                 Parameters:
                 zone_name (isc.dns.Name) name to format
                 zone_class (isc.dns.RRClass) class to format
                 """
                 return zone_name.to_text(True) + '/' + str(zone_class)
 }}}

 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
 }}}

 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])
 }}}

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