On 29.6.2016 18:48, Lenka Doudova wrote: > > > On 06/27/2016 11:05 AM, Lenka Doudova wrote: >> >> >> On 06/27/2016 10:33 AM, Martin Babinsky wrote: >>> On 06/27/2016 10:28 AM, Petr Spacek wrote: >>>> On 27.6.2016 10:26, Petr Spacek wrote: >>>>> On 27.6.2016 10:18, Martin Babinsky wrote: >>>>>> On 06/27/2016 10:04 AM, Petr Vobornik wrote: >>>>>>> On 06/27/2016 09:42 AM, Lenka Doudova wrote: >>>>>>>> Hi! >>>>>>>> >>>>>>>> With newly created AD machines in Brno lab, existing trust tests fail >>>>>>>> on >>>>>>>> 'ipa dnsforwardzone-add' command claiming the zone is already present, >>>>>>>> as new AD domain is dom-221.idm.lab.eng.brq.redhat.com. >>>>>>>> >>>>>>>> To prevent these failures I prepared attached patch, that will still >>>>>>>> attempt to add the forward zone, but in case of non-zero return code >>>>>>>> will check the message if it says that the forward zone is already >>>>>>>> configured, and lets the tests continue, if it is so. >>>>>>>> >>>>>>>> >>>>>>>> Lenka >>>>>>>> >>>>>>> >>>>>>> >>>>>>> Current approach expects that every error of ipa dnsforward-add here >>>>>>> will mean that the zone exists. So it might hide other issues - not very >>>>>>> good. >>>>>>> >>>>>>> On the other hand it is not very robust to parse error message. >>>>>>> >>>>>>> Question for general audience: What do you think if IPA client's exit >>>>>>> status would be the IPA error code instead of "1" for every error. E.g. >>>>>>> in DuplicateEntry case it's 4002. >>>>>>> >>>>>>> Btw, this is not a NACK. >>>>>>> >>>>>> >>>>>> Well AFAIK the exit status on POSIX systems is encoded into a single >>>>>> byte so >>>>>> you cannot have the return value greater that 255. We would have to >>>>>> devise >>>>>> some mapping between our XMLRPC status codes and subprocess return codes. >>>>>> >>>>>> Some of our exceptions have defined return values outside plain '1', e.g. >>>>>> NotFound has return value of 2. It would be possible to extend this >>>>>> concept on >>>>>> other common errors. >>>>> >>>>> Even more importantly, the forward zone is completely unnecessary because >>>>> DNS >>>>> when DNS is set up properly. I would simply remove the dnsforwardzone-add. >>>>> >>>> Grr, I meant this: >>>> Even more importantly, the forward zone is completely unnecessary when DNS >>>> is >>>> set up properly. I would simply remove the dnsforwardzone-add. >>>> >>> +1, our tests should not fiddle with the provisioned environment as much as >>> they sometimes do. >>> >> Well, I have nothing against removing it completely, but left it there just >> because with previous AD machines with "wild" domains it was necessary. >> Looking at the code, your suggestion would probably mean to entirely remove >> method configure_dns_for_trust from ipatests/test_integration/tasks.py, >> right? I'll have to verify this won't break anything else. >> >> Lenka >> > Hi, > > to get back to this issue: do we really want to have the DNS configuration > method removed? I mean, we no longer need it for our CI tests, with new AD VMs > it works without it, but should somebody else with different setup run these > tests, they could experience failures because their AD domain would not be > configured in DNS by default and the test would no longer provide that > configuration. It doesn't feel right to delete something we needed before but > don't need anymore, in case somebody else is depending on the same > configuration. But of course, I'll abide by your counsel. > In case the call on DNS configuration method really is removed, should I > remove even it's definition? It's not used anywhere else, so it would be quite > logical.
Feel free to keep empty method around as a "hook" for other people. The important thing is that it should do nothing by default. -- Petr^2 Spacek -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
