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

 What happened to the branch? Was it rebased? Why? Why didn't the ticket
 say so?

 Replying to [comment:16 pselkirk]:
 > > 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.

 This works.

 But my idea was more like having it as variable inside the mock finder
 object. A new one would be created each time, so overwriting it would not
 influence other tests. Then it would not look like a global constant.
 Anyway, this is good.

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

 Existence of some bad code is not a good argument for adding more ;-). I
 guess most of the zone manager was written in early stage of development
 when many developers were unfamiliar with python and were used to C.

 > > 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 don't have a good feeling even when we repeat one-line functions in
 multiple places. So I would prefer the one-line function to move there.
 Or, not define the function and just provide the result there. I think
 there was just one place that used it.

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

 There it makes sense. If you add new zone, the first time it is downloaded
 it has no SOA. I'm not completely sure about the zone manager, but if the
 timeout on zone was triggered, then it sounds like there already was SOA
 that could define the timeout in the first place. But as I said, I'm not
 sure how zone manager behaves if there's an empty zone.

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

 Well, we have the client list, that may contain multiple datasource
 clients. And a zone of the given name may live in multiple of them. In
 that case, the configuration of zone manager (once we think how it should
 look like, because that part will be probably changed) may contain name of
 the data source in which the zone to be updated should live. And the name
 will be passed in this call.

 But as I said, this is out of scope of the current ticket.

 The code would look good, but I can't compile it. It seems to be related
 to the errors in master.

 {{{
 ncr_msg.o: In function
 `boost::date_time::month_formatter<boost::gregorian::greg_month,
 boost::date_time::iso_format<char>,
 char>::format_month(boost::gregorian::greg_month const&,
  std::ostream&)':
 /usr/include/boost/date_time/date_formatting.hpp:44: undefined reference
 to `boost::gregorian::greg_month::as_short_string() const'
 /usr/include/boost/date_time/date_formatting.hpp:49: undefined reference
 to `boost::gregorian::greg_month::as_long_string() const
 }}}

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