Re: [Freeipa-devel] [PATCH 0022][Tests] Prevent trust test failures cause by adding duplicate DNS forward zone

[Martin Basti]

On 26.07.2016 12:17, Lenka Doudova wrote:



On 06/30/2016 01:14 PM, Martin Basti wrote:



On 30.06.2016 12:58, Lenka Doudova wrote:



On 06/30/2016 12:51 PM, Petr Spacek wrote:

On 30.6.2016 12:32, Lenka Doudova wrote:


On 06/29/2016 07:49 PM, Petr Spacek wrote:

On 29.6.2016 18:52, Lenka Doudova wrote:

On 06/29/2016 06:51 PM, Petr Spacek wrote:

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.

So leave the method call, but erase method contents and let it 
just pass?

Fine with me. (List re-added.)


Ah, sorry for doing the wrong reply.
Anyway, fixed patch attached. I decided to do as you suggested - 
the original
DNS configuring function is now empty, I modified the comment to 
explain
significance of this strange thing. I also changed patch title to 
better

reflect proposed changes.

ACK if it passes your tests.

Yes, I've had no problems running the tests since I started to use 
this.

Thanks.


Pushed to master: 1d9e1521c59a5b43c2322892ce5cbe8cceff2790


Hi,

I just realized the same problem occurs in 4.3 branch, but the 
original patch was pushed to master only, hence I ask for second review.
The originally pushed patch attached, should not need any 
modifications for ipa-4-3 branch.


Ticket: https://fedorahosted.org/freeipa/ticket/6133

Thanks,
Lenka



Pushed to ipa-4-3: 40b1459ad0299e95331699be9684682fca02a570

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

Re: [Freeipa-devel] [PATCH 0022][Tests] Prevent trust test failures cause by adding duplicate DNS forward zone

[Lenka Doudova]

On 06/30/2016 01:14 PM, Martin Basti wrote:



On 30.06.2016 12:58, Lenka Doudova wrote:



On 06/30/2016 12:51 PM, Petr Spacek wrote:

On 30.6.2016 12:32, Lenka Doudova wrote:


On 06/29/2016 07:49 PM, Petr Spacek wrote:

On 29.6.2016 18:52, Lenka Doudova wrote:

On 06/29/2016 06:51 PM, Petr Spacek wrote:

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.

So leave the method call, but erase method contents and let it 
just pass?

Fine with me. (List re-added.)


Ah, sorry for doing the wrong reply.
Anyway, fixed patch attached. I decided to do as you suggested - 
the original
DNS configuring function is now empty, I modified the comment to 
explain
significance of this strange thing. I also changed patch title to 
better

reflect proposed changes.

ACK if it passes your tests.


Yes, I've had no problems running the tests since I started to use this.
Thanks.


Pushed to master: 1d9e1521c59a5b43c2322892ce5cbe8cceff2790


Hi,

I just realized the same problem occurs in 4.3 branch, but the original 
patch was pushed to master only, hence I ask for second review.
The originally pushed patch attached, should not need any modifications 
for ipa-4-3 branch.


Ticket: https://fedorahosted.org/freeipa/ticket/6133

Thanks,
Lenka
From a4772a1b8cc31f0020c86acabbeb944c6d5269b7 Mon Sep 17 00:00:00 2001
From: Lenka Doudova 
Date: Thu, 30 Jun 2016 12:26:28 +0200
Subject: [PATCH] Tests: Remove DNS configuration from trust tests

Since DNS configuration is no longer needed for running trust tests, this method's contents are removed. Method is left empty as reference for others, should they have issues with DNS configuration.
---
 

Re: [Freeipa-devel] [PATCH 0022][Tests] Prevent trust test failures cause by adding duplicate DNS forward zone

[Martin Basti]

On 30.06.2016 12:58, Lenka Doudova wrote:



On 06/30/2016 12:51 PM, Petr Spacek wrote:

On 30.6.2016 12:32, Lenka Doudova wrote:


On 06/29/2016 07:49 PM, Petr Spacek wrote:

On 29.6.2016 18:52, Lenka Doudova wrote:

On 06/29/2016 06:51 PM, Petr Spacek wrote:

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.

So leave the method call, but erase method contents and let it 
just pass?

Fine with me. (List re-added.)


Ah, sorry for doing the wrong reply.
Anyway, fixed patch attached. I decided to do as you suggested - the 
original
DNS configuring function is now empty, I modified the comment to 
explain
significance of this strange thing. I also changed patch title to 
better

reflect proposed changes.

ACK if it passes your tests.


Yes, I've had no problems running the tests since I started to use this.
Thanks.


Pushed to master: 1d9e1521c59a5b43c2322892ce5cbe8cceff2790

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

Re: [Freeipa-devel] [PATCH 0022][Tests] Prevent trust test failures cause by adding duplicate DNS forward zone

[Lenka Doudova]

On 06/30/2016 12:51 PM, Petr Spacek wrote:

On 30.6.2016 12:32, Lenka Doudova wrote:


On 06/29/2016 07:49 PM, Petr Spacek wrote:

On 29.6.2016 18:52, Lenka Doudova wrote:

On 06/29/2016 06:51 PM, Petr Spacek wrote:

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.


So leave the method call, but erase method contents and let it just pass?

Fine with me. (List re-added.)


Ah, sorry for doing the wrong reply.
Anyway, fixed patch attached. I decided to do as you suggested - the original
DNS configuring function is now empty, I modified the comment to explain
significance of this strange thing. I also changed patch title to better
reflect proposed changes.

ACK if it passes your tests.


Yes, I've had no problems running the tests since I started to use this.
Thanks.

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

Re: [Freeipa-devel] [PATCH 0022][Tests] Prevent trust test failures cause by adding duplicate DNS forward zone

[Petr Spacek]

On 30.6.2016 12:32, Lenka Doudova wrote:
> 
> 
> On 06/29/2016 07:49 PM, Petr Spacek wrote:
>> On 29.6.2016 18:52, Lenka Doudova wrote:
>>>
>>> On 06/29/2016 06:51 PM, Petr Spacek wrote:
 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.

>>> So leave the method call, but erase method contents and let it just pass?
>> Fine with me. (List re-added.)
>>
> Ah, sorry for doing the wrong reply.
> Anyway, fixed patch attached. I decided to do as you suggested - the original
> DNS configuring function is now empty, I modified the comment to explain
> significance of this strange thing. I also changed patch title to better
> reflect proposed changes.

ACK if it passes your tests.

-- 
Petr^2 Spacek

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to 

Re: [Freeipa-devel] [PATCH 0022][Tests] Prevent trust test failures cause by adding duplicate DNS forward zone

[Oleg Fayans]

On 06/30/2016 12:41 PM, Lenka Doudova wrote:
> 
> 
> On 06/30/2016 12:32 PM, Lenka Doudova wrote:
>>
>>
>> On 06/29/2016 07:49 PM, Petr Spacek wrote:
>>> On 29.6.2016 18:52, Lenka Doudova wrote:

 On 06/29/2016 06:51 PM, Petr Spacek wrote:
> 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.
>
 So leave the method call, but erase method contents and let it just
 pass?
>>> Fine with me. (List re-added.)
>>>
>> Ah, sorry for doing the wrong reply.
>> Anyway, fixed patch attached. I decided to do as you suggested - the
>> original DNS configuring function is now empty, I modified the 

Re: [Freeipa-devel] [PATCH 0022][Tests] Prevent trust test failures cause by adding duplicate DNS forward zone

[Lenka Doudova]

On 06/30/2016 12:32 PM, Lenka Doudova wrote:



On 06/29/2016 07:49 PM, Petr Spacek wrote:

On 29.6.2016 18:52, Lenka Doudova wrote:


On 06/29/2016 06:51 PM, Petr Spacek wrote:

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.

So leave the method call, but erase method contents and let it just 
pass?

Fine with me. (List re-added.)


Ah, sorry for doing the wrong reply.
Anyway, fixed patch attached. I decided to do as you suggested - the 
original DNS configuring function is now empty, I modified the comment 
to explain significance of this strange thing. I also changed patch 
title to better reflect proposed changes.


Lenka



NACK the previous one, forgot PEP8. New patch attached.

Lenka
From a4772a1b8cc31f0020c86acabbeb944c6d5269b7 Mon Sep 17 00:00:00 2001
From: Lenka Doudova 
Date: Thu, 30 Jun 2016 12:26:28 +0200
Subject: [PATCH] Tests: Remove DNS configuration from trust tests

Since DNS configuration is no longer needed for running trust tests, this method's contents are removed. Method is left empty as reference for others, should they have issues with DNS configuration.
---
 ipatests/test_integration/tasks.py | 44 --
 1 file changed, 4 insertions(+), 40 deletions(-)

diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
index 38218fa709c2c220d5fea98a092b55e995d48d77..5be7cdae3ac777bbf0fc52e6c511969e9fabcd72 100644
--- a/ipatests/test_integration/tasks.py
+++ b/ipatests/test_integration/tasks.py
@@ -436,47 +436,11 @@ def install_adtrust(host):
 
 def configure_dns_for_trust(master, ad):
 """
-This configures DNS on IPA master according to the relationship of the
-  

Re: [Freeipa-devel] [PATCH 0022][Tests] Prevent trust test failures cause by adding duplicate DNS forward zone

[Lenka Doudova]

On 06/29/2016 07:49 PM, Petr Spacek wrote:

On 29.6.2016 18:52, Lenka Doudova wrote:


On 06/29/2016 06:51 PM, Petr Spacek wrote:

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.


So leave the method call, but erase method contents and let it just pass?

Fine with me. (List re-added.)


Ah, sorry for doing the wrong reply.
Anyway, fixed patch attached. I decided to do as you suggested - the 
original DNS configuring function is now empty, I modified the comment 
to explain significance of this strange thing. I also changed patch 
title to better reflect proposed changes.


Lenka
From c931a57aa4a0162b7f28d2fe0e01d43c8c95c146 Mon Sep 17 00:00:00 2001
From: Lenka Doudova 
Date: Thu, 30 Jun 2016 12:26:28 +0200
Subject: [PATCH] Tests: Remove DNS configuration from trust tests

Since DNS configuration is no longer needed for running trust tests, this method's contents are removed. Method is left empty as reference for others, should they have issues with DNS configuration.
---
 ipatests/test_integration/tasks.py | 44 --
 1 file changed, 4 insertions(+), 40 deletions(-)

diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
index 38218fa709c2c220d5fea98a092b55e995d48d77..1aa7772a1b59b4ad78bd3b3f653c8782ae6041de 100644
--- a/ipatests/test_integration/tasks.py
+++ b/ipatests/test_integration/tasks.py
@@ -436,47 +436,11 @@ def install_adtrust(host):
 
 def configure_dns_for_trust(master, ad):
 """
-This configures DNS on IPA master according to the relationship of the
-IPA's and AD's domains.
+This method is intentionally left empty. Originally it served for DNS 
+configuration on IPA master according to the 

Re: [Freeipa-devel] [PATCH 0022][Tests] Prevent trust test failures cause by adding duplicate DNS forward zone

[Petr Spacek]

On 29.6.2016 18:52, Lenka Doudova wrote:
> 
> 
> On 06/29/2016 06:51 PM, Petr Spacek wrote:
>> 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.
>>
> So leave the method call, but erase method contents and let it just pass?

Fine with me. (List re-added.)

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

Re: [Freeipa-devel] [PATCH 0022][Tests] Prevent trust test failures cause by adding duplicate DNS forward zone

[Petr Spacek]

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

Re: [Freeipa-devel] [PATCH 0022][Tests] Prevent trust test failures cause by adding duplicate DNS forward zone

[Lenka Doudova]

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

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

Re: [Freeipa-devel] [PATCH 0022][Tests] Prevent trust test failures cause by adding duplicate DNS forward zone

[Martin Babinsky]

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.


--
Martin^3 Babinsky

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

Re: [Freeipa-devel] [PATCH 0022][Tests] Prevent trust test failures cause by adding duplicate DNS forward zone

[Petr Spacek]

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.

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

Re: [Freeipa-devel] [PATCH 0022][Tests] Prevent trust test failures cause by adding duplicate DNS forward zone

[Petr Spacek]

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.

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

Re: [Freeipa-devel] [PATCH 0022][Tests] Prevent trust test failures cause by adding duplicate DNS forward zone

[Petr Vobornik]

On 06/27/2016 10:10 AM, Lenka Doudova 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.
> If I understand your comment correctly, you think that the patch would
> pass ANY dnsforwardzone-add error and being OK and continue, right?
> That's not intended behaviour - I have an assertion there that checks
> that it's really the 'correct' error:
> 
> assert "already exists in DNS" in result.stderr_text
> 
> So any other error should still prevent continuing in tests.
> 

Ah, good, I've overlooked that.


-- 
Petr Vobornik

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

Re: [Freeipa-devel] [PATCH 0022][Tests] Prevent trust test failures cause by adding duplicate DNS forward zone

[Martin Babinsky]

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.


--
Martin^3 Babinsky

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

Re: [Freeipa-devel] [PATCH 0022][Tests] Prevent trust test failures cause by adding duplicate DNS forward zone

[Lenka Doudova]

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.
If I understand your comment correctly, you think that the patch would 
pass ANY dnsforwardzone-add error and being OK and continue, right? 
That's not intended behaviour - I have an assertion there that checks 
that it's really the 'correct' error:


assert "already exists in DNS" in result.stderr_text

So any other error should still prevent continuing in tests.



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.
Personally I think it would be nice to have DuplicateEntry error rather 
the just "1" in this case. Even for testing purposes I believe it would 
be better than bunch of asserts.


Btw, this is not a NACK.


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

Re: [Freeipa-devel] [PATCH 0022][Tests] Prevent trust test failures cause by adding duplicate DNS forward zone

[Petr Vobornik]

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

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

[Freeipa-devel] [PATCH 0022][Tests] Prevent trust test failures cause by adding duplicate DNS forward zone

[Lenka Doudova]

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

From 33761de592e867d63665ebe974dbec7c29294367 Mon Sep 17 00:00:00 2001
From: Lenka Doudova 
Date: Mon, 27 Jun 2016 09:29:31 +0200
Subject: [PATCH] Tests: Prevent trust test failures caused by adding duplicate
 DNS forward zone

When a DNS forward zone is already configured, the 'ipa dnsforwardzone-add' command fails and prevents running trust test. New check is added that will allow tests to continue even if the command raises error caused by attempt to add already existing zone.
---
 ipatests/test_integration/tasks.py | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
index 38218fa709c2c220d5fea98a092b55e995d48d77..662fa3b73b9d1bea597199cad4d438e1ab339b01 100644
--- a/ipatests/test_integration/tasks.py
+++ b/ipatests/test_integration/tasks.py
@@ -473,11 +473,13 @@ def configure_dns_for_trust(master, ad):
 master.run_command(['ipa', 'dnszone-mod', master.domain.name,
 '--allow-transfer', ad.ip])
 else:
-master.run_command(['ipa', 'dnsforwardzone-add', ad.domain.name,
-'--forwarder', ad.ip,
-'--forward-policy', 'only',
-])
-
+result = master.run_command(['ipa', 'dnsforwardzone-add',
+ ad.domain.name,
+ '--forwarder', ad.ip,
+ '--forward-policy', 'only'
+ ], raiseonerr=False)
+if result.returncode != 0:
+assert "already exists in DNS" in result.stderr_text
 
 def establish_trust_with_ad(master, ad, extra_args=()):
 """
-- 
2.7.4

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