BIND 10 #1414: Zonemgr/secondary_zones[]/class required

[BIND 10 Development]

#1414: Zonemgr/secondary_zones[]/class required
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:         |             Milestone:
  defect                             |  Sprint-20111220
                   Priority:  major  |            Resolution:
                  Component:         |             Sensitive:  0
  configuration                      |           Sub-Project:  Core
                   Keywords:         |  Estimated Difficulty:  4
            Defect Severity:  N/A    |           Total Hours:  2
Feature Depending on Ticket:  none   |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 '''zonemgr'''

 - not really for this branch, but I think we should rather check the
   given name is a valid domain name (same for the RR class, btw) by
   converting it into a Name object, then dump it to text (lower-cased
   with ending period):
 {{{#!python
                     # Be tolerant to sclerotic users who forget the final
 dot
                     if name[-1] != '.':
                         name = name + '.'
 }}}
   xfrout does this, btw.  See _get_transfer_acl().  It can be deferred
   to a separate ticket, though.  I mentioned it simply because I
   noticed it while I reading the patch.

 '''zonemgr_test'''

 - I'd integrate FakeConfig and FakeCCSession (and "MyCCSession"
   defined and used somewhere else in the test).  See the attached diff
   for concrete ideas.  The point is:
   - eliminate hardcoding as much as possible
   - integrate functionality FakeConfig and MyCCSession into
     FakeCCSession, thus eliminating the need for the former(s)
     completely.

 - FakeCCSession constructor: this doesn't seem to be necessary:
 {{{#!python
         self.config = FakeConfig()
 }}}
  (in the sample patch I removed this line, too)

 - (this is not really for this branch) While thinking about this further
   refactoring, I realized this part didn't seem to make sense:
 {{{#!python
         # This one does not exist
         config['secondary_zones'] = \
             zone_list_from_name_classes(["example.net", "CH"])
         self.zone_refresh.update_config_data(config, self.cc_session)
         self.assertFalse(("example.net.", "CH") in
                          self.zone_refresh._zonemgr_refresh_info)
         # Simply skip loading soa for the zone, the other configs should
 be updated successful
         self.assertFalse(("example.net.", "IN") in
                          self.zone_refresh._zonemgr_refresh_info)
 }}}
   (this is after refactoring, but I believe it preserves the test
   semantics)  First, I suspect `["example.net", "CH"]` should have
   been `[("example.net", "CH")]`.  But then the subsequent test failed.
   I couldn't even be sure what exactly these sequences tried to test,
   so I simply kept the original semantics even though it didn't make
   sense to me.

 '''Others'''

 - I've made a couple of minor editorial cleanups.
 - I believe we need a changelog entry for this.

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