BIND 10 #1378: Configuration of #213

[BIND 10 Development]

#1378: Configuration of #213
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  vorner                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20111122
                  Component:  Boss   |            Resolution:
  of BIND                            |             Sensitive:  0
                   Keywords:         |           Sub-Project:  Core
            Defect Severity:  N/A    |  Estimated Difficulty:  0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jinmei


Comment:

 Hello

 Replying to [comment:5 jinmei]:
 > '''bind10_src.py.in'''
 > - read_bind10_config and start_all_processes are now both sufficiently
 >   small and could be combined into a single method.  The only reason
 >   we don't do this seems to be the convenience for tests, and, if
 >   that's the reason I'd explicitly add a note to read_bind10_config
 >   (otherwise someone would try "refactoring" the code later).  Also,
 >   if that's the reason we might want to make read_bind10_config
 >   "protected" by renaming it to `_read_bind10_config`.

 Well, I didn't really think about it until you pointed it out, but yes,
 it's good for tests. I added the note.

 > '''unit tests'''
 > - MockBob.start_simple(): why this change?
 > {{{
 > -                    'b10-xfrin': self.start_xfrin,
 > }}}
 >   (why was xfrin removed while keeping others?)

 Because xfrin is currently special component (until we do something with
 the workaround with environment variables), so start_simple is not used to
 start it (and in fact shouldn't, so if it was used by accident, this would
 complain).

 It should be readded once the workaround is not needed and it's no longer
 special.

 > - bind10_test: I don't think removing
 >   test_start(none/auth/resolver/both) is really correct (maybe except
 >   for none), because in test_config_start we only check the code path
 >   from config_handler while test_start variants check the one from
 >   start_all_processes (and testing these cases shouldn't be difficult
 >   anyway).

 OK, I added them back (or something that should be equivalent with them).

 > - test_config_start_once: what's the purpose of this addition?
 > {{{
 > +        bob._BoB_started = True
 > }}}
 >   (same for test_start_dhcp, etc)

 Since the boss ignores configuration updates until it is fully started and
 it uses this variable to know it was started. We don't do the real
 startup, so we need to fake it.

 > - TestBossComponents: is there anything that has changed from the
 >   original trac213 tests?  (I'm asking whether I need to review this
 >   again)

 I copy-pasted it from the original version. I'm not 100% sure if I needed
 to update any variable names which might have changed in between, but they
 are the same otherwise.

 > '''system tests'''
 > I guess this hack is due to a bug already fixed in master (forgot the
 > ticket number):
 > {{{
 > +echo 'config add Boss/components x
 > +config remove Boss/components x
 > }}}
 > I'd rather merge that fix here and remove this kludge now.

 Yes, I know about that one. I didn't want to bring in another merge which
 would make the review harder. I want to remove it at the merge with master
 (or, I'd propose waiting at last until the branch stabilizes, just because
 of the reviews). I don't want this to exist in master.

 Thanks.

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