BIND 10 #811: Put TSIG field into list of zones we should manage.

[BIND 10 Development]

#811: Put TSIG field into list of zones we should manage.
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  stephen                            |                Status:  reviewing
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20110517
                   Priority:         |            Resolution:
  blocker                            |             Sensitive:  0
                  Component:         |           Sub-Project:  DNS
  Unclassified                       |  Estimated Difficulty:  2.0
                   Keywords:         |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:  tsig   |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => jinmei


Comment:

 Replying to [comment:14 jinmei]:
 > Replying to [comment:13 jelte]:
 >
 > === Xfrin ===
 >
 > <snip>
 >
 > One quick-hack fix would be to convert the name to down-cased one
 > within _check_zone_name:
 >
 > (and I guess we need a test case for it)
 >
 > Or, if we declare this is quite minor and less likely to happen in
 > practice (and defer everything to the later ticket), I'm also okay
 > with that.
 >

 easy and minor enough to fix now (this is where the earlier smallish
 refactors start to pay off :))

 > > XfrinConfigException was a bad name, as it was really only about zone
 > > information data, so I changed the exception to XfrinZoneInfoException
 > > ('something is wrong with the given zone information').  And the
 _parse
 > > function now calls the same check function.
 >
 > And, with that change, I think we can really remove the redundancy.
 > I'm attaching a proposed patch.
 >

 ok, thanks, applied it

 > > >  - _parse_zone_name_and_class: I also noticed you use 'not XXX' to
 > > >    mean "not specified", rather than "is None".  This happens when
 > > >    the given string is actually "", rather than "unspecified".  But
 > > >    since these will be rejected via the Name()/RRClass()
 constructors,
 > >
 > > according to git, that's your code, btw :)
 >
 > Oh, is it?  I admit I myself sometimes am confused about None and not:-)
 >

 i shan't say i never make the mistake (and i certainly read over it
 easily), just happens that i was explaining this difference to another dev
 last week too (so the 'you' got me a bit there ;))

 > Comments on the revised code follow:
 >
 > '''xfrin_test.py'''
 >  - in test_command_handler_zones(), zonesN should better be renamed to
 >    "config1" or "xfrin_config1", etc, because now they are not only
 >    for zones.
 >

 ok

 > '''xfrin.py.in'''
 >
 > - XfrinZoneInfoException: "when a command does not have the required
 >   or bad arguments" doesn't seem to parse reasonably. "when a command
 >   does not have a required argument or has bad arguments"?
 >

 ok

 > - in set_master_port, should we worry about if the default spec value
 >   is out of range for a valid port number (e.g. >= 2^16)?
 >   same for set_zone_class. (Sorry I should have pointed these before.)
 >   I'm okay with ignoring it as an unlikely scenario (at least for now).
 >

 hmm. let's defer that too, not that i'm lazy (well ok I am, but i may have
 a reason here:), but this makes me think that maybe we do need a set of
 generalized check-calls like is_valid_port_number, etc. Preferably ones
 that do not raise, but do convert to specific type if necessary (and do
 provide the error if they can't).

 > - Zoneinfo.set_name: is this TODO still active?  From a quick check it
 >   seems we can safely remove this line:
 > {{{
 >         self.name_str = name_str
 > }}}
 >

 oh hey indeed, missed that one

 > '''Other points'''
 > - xfrout tests failed for me.  We could fix it, but based on the
 >   discussion at the sprint planning, I'd rather suggest deferring the
 >   whole xfrout part to a separate ticket and concentrate on finishing
 >   xfrin.  at the moment I've not closely looked at the xfrout code.

 ok, i reverted the changes there (btw i suspect your failed test was a
 missing dependency that caused the configure-time-created test file to not
 be recreated)

 > - my previous comment about missing test for get_default_value()
 >   doesn't seem to be addressed.

 woops, sorry, added

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