Re: [Freeipa-devel] [PATCH 0394, 0379 - 0393] Winter code cleanup

[Jan Cholasta]

On 22.12.2015 15:18, Martin Basti wrote:



On 22.12.2015 13:43, Martin Basti wrote:



On 22.12.2015 08:06, Jan Cholasta wrote:

On 17.12.2015 14:18, Martin Basti wrote:



On 17.12.2015 07:54, Jan Cholasta wrote:

Hi,

On 17.12.2015 02:11, Martin Basti wrote:

Hello,
following patches cleanup the code and add checks to pylint to
avoid the
mess again

Patches: 379-381:
the most important patches, they cleanup imports

Patch 382:
enables various pylint checks, we may reduce the list of check if
needed.


Would it be possible to turn this into a blacklist of disabled
warnings rather than a whitelist of enabled warnings?



Patches 383 - 393:
remove minor issues from code, and enable particular checks
Feel free to nack patches/checks that should not be there.

Please apply patches in order.

Martin^2




Honza


Updated patches attached.


Patch 379: ACK


Patch 380:

1) This patch needs to be rebased.


2) make-lint is reporting this to me:

* Module ipatests.test_cmdline.test_ipagetkeytab
ipatests/test_cmdline/test_ipagetkeytab.py:30: [W0611(unused-import),
] Unused import nose)
ipatests/test_cmdline/test_ipagetkeytab.py:34: [W0611(unused-import),
] Unused DN imported from ipapython.dn)
* Module ipatests.test_cmdline.test_help
ipatests/test_cmdline/test_help.py:21: [W0611(unused-import), ]
Unused import contextlib)
ipatests/test_cmdline/test_help.py:23: [W0611(unused-import), ]
Unused assert_raises imported from nose.tools)
* Module ipatests.test_cmdline.test_cli
ipatests/test_cmdline/test_cli.py:11: [W0611(unused-import), ] Unused
API_VERSION imported from ipapython.version)
* Module ipaplatform.services
ipaplatform/services.py:59: [W0611(unused-import), ] Unused
timedate_services imported from ipaplatform.redhat.services)
* Module ipaplatform.rhel.services
ipaplatform/rhel/services.py:59: [W0611(unused-import), ] Unused
timedate_services imported from ipaplatform.redhat.services)
* Module ipaplatform.redhat.services
ipaplatform/redhat/services.py:306: [W0611(unused-import), ] Unused
timedate_services imported from ipaplatform.base.services)
* Module ipaplatform.fedora.services
ipaplatform/fedora/services.py:59: [W0611(unused-import), ] Unused
timedate_services imported from ipaplatform.redhat.services)
* Module ipalib.setup
ipalib/setup.py:28: [W0611(unused-import), ] Unused import
distutils.sysconfig)

Note that the "from ipaplatform.${PLATFORM}.services import
timedate_services" in services.py should be rewritten to
"timedate_services = ${PLATFORM}_services.timedate_services" to fix
this.


Patch 381: ACK


Patch 382:

Nitpick: according to , the order
of message categories is F, E, W, C, R from the most severe to the
least severe (it does not mention I, but I think it should be last,
after R), IMO we should keep this order here as well for clarity.
(Don't worry, doing this should not require rebasing the following
patches.)


Patch 383 - 387: ACK


Patch 388:

IMO it would be better to rewrite this:

[(t.id, t.options) for t in doc.getKeyPackages()]  #
pylint: disable=expression-not-assigned

into this:

for t in doc.getKeyPackages():
t._PSKCKeyPackage__process()

rather than disabling the check.


Patch 389:

All this patch does is that it enables a check globally and then
disables it everywhere locally.

IMO the patch should be retired for now, or make-lint should be
taught to automatically skip the check inside TestCase.assertRaises()
context.


Patch 390: ACK


Patch 391:

default_from uses argument names of the specified function as param
names from which to create the default.

These changes break default_from:

-default_from=lambda name_from_ip:
_reverse_zone_name(name_from_ip),
+default_from=_reverse_zone_name,

-default_from=lambda idnsname:
default_zone_update_policy(idnsname),
+default_from=default_zone_update_policy,

This change adds dependency on non-existent param:

-default_from=lambda: krb_utils.get_principal(),
+default_from=krb_utils.get_principal,

The result of this check is misleading for default_from, so IMO the
patch should be retired for now, or make-lint should be taught to
automatically disable the check for the default_from argument in
param definitions.


Patch 392:

I think the existing occurences of exec()/eval() should be removed
before this check can be enabled.


Patch 393: ACK


I would like to see a patch which enables the useless-suppression
info message, so that we can catch dangling "# pylint: disable="
comments (there are some in the code).

(Also, it would be nice to finally rewrite make-lint to pylint config
file + plugins.)


Updated patches attached, please apply patch 394 first.

Patches 389, 391, 392 have been removed, issues will be addressed later.


OK.



useless-suppression, unbalanced-tuple-unpacking,
unpacking-non-sequence, 

Re: [Freeipa-devel] ipa-devel repos on jdennis.fedorapeople.org

[Petr Vobornik]

On 12/22/2015 05:19 PM, Petr Spacek wrote:

On 22.12.2015 17:18, John Dennis wrote:

On 12/22/2015 09:50 AM, Petr Spacek wrote:

John, the machines which used to generate the repos are basically dead now.

Could you remove the directories and replace them with a README with sentence
that the repos were discontinued, please?


I'd be happy to, but shouldn't the README contain a pointer to the preferred
repo/copr whatever it is now? What is it?


AFAIK we do not have nightly builds yet :-(

Petr, could we have some nightly or post-commit builds in Fedora Copr?



It is easy to setup a vm for nightly copr builds.

We should investigate a possibility of getting a project copr account. 
It is not good to build everything in mkosek's namespace.

--
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] ipa-devel repos on jdennis.fedorapeople.org

[Petr Spacek]

On 22.12.2015 17:18, John Dennis wrote:
> On 12/22/2015 09:50 AM, Petr Spacek wrote:
>> John, the machines which used to generate the repos are basically dead now.
>>
>> Could you remove the directories and replace them with a README with sentence
>> that the repos were discontinued, please?
> 
> I'd be happy to, but shouldn't the README contain a pointer to the preferred
> repo/copr whatever it is now? What is it?

AFAIK we do not have nightly builds yet :-(

Petr, could we have some nightly or post-commit builds in Fedora Copr?

-- 
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] ipa-devel repos on jdennis.fedorapeople.org

[John Dennis]

On 12/22/2015 09:50 AM, Petr Spacek wrote:

John, the machines which used to generate the repos are basically dead now.

Could you remove the directories and replace them with a README with sentence
that the repos were discontinued, please?


I'd be happy to, but shouldn't the README contain a pointer to the 
preferred repo/copr whatever it is now? What is it?



--
John

--
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] 811 performance: faster DN implementation

[Petr Spacek]

On 14.4.2015 19:32, Petr Vobornik wrote:
> 
> Pushed to master: 11bd9d96f191066f7ba760549f00179c128a9787
> 
>>
>> Please be so kind and fix naming. AFAIK the patch refers to 'openldap' DN
>> format but in fact it is python-ldap-ishm. It will surely confuse next
>> generation of FreeIPA developers :-)
>>
> 
> Will be in separate patch.

Petr, Santa found out that you did not send the patch! You are risking
Christmas without any presents ... :-)

-- 
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 0394, 0379 - 0393] Winter code cleanup

[Petr Spacek]

On 22.12.2015 15:18, Martin Basti wrote:
> 
> 
> On 22.12.2015 13:43, Martin Basti wrote:
>>
>>
>> On 22.12.2015 08:06, Jan Cholasta wrote:
>>> On 17.12.2015 14:18, Martin Basti wrote:


 On 17.12.2015 07:54, Jan Cholasta wrote:
> Hi,
>
> On 17.12.2015 02:11, Martin Basti wrote:
>> Hello,
>> following patches cleanup the code and add checks to pylint to avoid the
>> mess again
>>
>> Patches: 379-381:
>> the most important patches, they cleanup imports
>>
>> Patch 382:
>> enables various pylint checks, we may reduce the list of check if
>> needed.
>
> Would it be possible to turn this into a blacklist of disabled
> warnings rather than a whitelist of enabled warnings?
>
>>
>> Patches 383 - 393:
>> remove minor issues from code, and enable particular checks
>> Feel free to nack patches/checks that should not be there.
>>
>> Please apply patches in order.
>>
>> Martin^2
>>
>>
>
> Honza
>
 Updated patches attached.
>>>
>>> Patch 379: ACK
>>>
>>>
>>> Patch 380:
>>>
>>> 1) This patch needs to be rebased.
>>>
>>>
>>> 2) make-lint is reporting this to me:
>>>
>>> * Module ipatests.test_cmdline.test_ipagetkeytab
>>> ipatests/test_cmdline/test_ipagetkeytab.py:30: [W0611(unused-import), ]
>>> Unused import nose)
>>> ipatests/test_cmdline/test_ipagetkeytab.py:34: [W0611(unused-import), ]
>>> Unused DN imported from ipapython.dn)
>>> * Module ipatests.test_cmdline.test_help
>>> ipatests/test_cmdline/test_help.py:21: [W0611(unused-import), ] Unused
>>> import contextlib)
>>> ipatests/test_cmdline/test_help.py:23: [W0611(unused-import), ] Unused
>>> assert_raises imported from nose.tools)
>>> * Module ipatests.test_cmdline.test_cli
>>> ipatests/test_cmdline/test_cli.py:11: [W0611(unused-import), ] Unused
>>> API_VERSION imported from ipapython.version)
>>> * Module ipaplatform.services
>>> ipaplatform/services.py:59: [W0611(unused-import), ] Unused
>>> timedate_services imported from ipaplatform.redhat.services)
>>> * Module ipaplatform.rhel.services
>>> ipaplatform/rhel/services.py:59: [W0611(unused-import), ] Unused
>>> timedate_services imported from ipaplatform.redhat.services)
>>> * Module ipaplatform.redhat.services
>>> ipaplatform/redhat/services.py:306: [W0611(unused-import), ] Unused
>>> timedate_services imported from ipaplatform.base.services)
>>> * Module ipaplatform.fedora.services
>>> ipaplatform/fedora/services.py:59: [W0611(unused-import), ] Unused
>>> timedate_services imported from ipaplatform.redhat.services)
>>> * Module ipalib.setup
>>> ipalib/setup.py:28: [W0611(unused-import), ] Unused import
>>> distutils.sysconfig)
>>>
>>> Note that the "from ipaplatform.${PLATFORM}.services import
>>> timedate_services" in services.py should be rewritten to "timedate_services
>>> = ${PLATFORM}_services.timedate_services" to fix this.
>>>
>>>
>>> Patch 381: ACK
>>>
>>>
>>> Patch 382:
>>>
>>> Nitpick: according to , the order of
>>> message categories is F, E, W, C, R from the most severe to the least
>>> severe (it does not mention I, but I think it should be last, after R), IMO
>>> we should keep this order here as well for clarity. (Don't worry, doing
>>> this should not require rebasing the following patches.)
>>>
>>>
>>> Patch 383 - 387: ACK
>>>
>>>
>>> Patch 388:
>>>
>>> IMO it would be better to rewrite this:
>>>
>>> [(t.id, t.options) for t in doc.getKeyPackages()]  # pylint:
>>> disable=expression-not-assigned
>>>
>>> into this:
>>>
>>> for t in doc.getKeyPackages():
>>> t._PSKCKeyPackage__process()
>>>
>>> rather than disabling the check.
>>>
>>>
>>> Patch 389:
>>>
>>> All this patch does is that it enables a check globally and then disables
>>> it everywhere locally.
>>>
>>> IMO the patch should be retired for now, or make-lint should be taught to
>>> automatically skip the check inside TestCase.assertRaises() context.
>>>
>>>
>>> Patch 390: ACK
>>>
>>>
>>> Patch 391:
>>>
>>> default_from uses argument names of the specified function as param names
>>> from which to create the default.
>>>
>>> These changes break default_from:
>>>
>>> -default_from=lambda name_from_ip:
>>> _reverse_zone_name(name_from_ip),
>>> +default_from=_reverse_zone_name,
>>>
>>> -default_from=lambda idnsname:
>>> default_zone_update_policy(idnsname),
>>> +default_from=default_zone_update_policy,
>>>
>>> This change adds dependency on non-existent param:
>>>
>>> -default_from=lambda: krb_utils.get_principal(),
>>> +default_from=krb_utils.get_principal,
>>>
>>> The result of this check is misleading for default_from, so IMO the patch
>>> should be retired for now, or make-lint should be taught to automatically
>>> disable the check for the default_from argument in param defin

Re: [Freeipa-devel] [PATCH 0069] Add 'review' target for make

[Petr Spacek]

On 16.12.2015 12:24, Martin Kosek wrote:
> On 12/16/2015 12:01 PM, Petr Spacek wrote:
>> On 16.12.2015 11:15, Martin Kosek wrote:
>>> On 12/16/2015 10:02 AM, Petr Spacek wrote:
 On 16.12.2015 09:53, Jan Cholasta wrote:
> On 16.12.2015 09:45, Petr Spacek wrote:
>> On 11.12.2015 15:50, Jan Cholasta wrote:
>>> Hi,
>>>
>>> On 10.12.2015 18:04, Petr Spacek wrote:
 On 9.12.2015 15:30, Petr Spacek wrote:
> Hello,
>
> this patch automates some of sanity checks proposed by Petr Vobornik.
>
> 'make review' should be used in root of clean Git tree which has 
> patches
> under
> review applied.
>
> Magic in review.sh attempts to detect nearest remote branch which can 
> be
> used
> as diff base for review. Please see review.sh for further details.

 And here is the patch! :-)
>>>
>>> Nice, but I would rather see this in ipatool
>>> (). Or is there any obvious 
>>> benefit
>>> in having this in freeipa itself that I'm missing?
>>
>> For me the obvious benefit is:
>> git clone
>> git am
>> make review
>>
>> Done.
>>
>> No need to find & learn other tool, no risk of using wrong version of 
>> the tool
>> to wrong version of source tree etc.
>
> AFAIK all IPA developers are supposed to use ipatool, and it already has a

 Good to know. How does a newcomer learn about it? Honestly I never used
 ipatool (or not even cloned it).
>>>
>>> ipatool targets rather upstream members with write access, so they are 
>>> hardly
>>> newcomers.
>>
>> I though that we want to make it easy to contribute, so why are you talking
>> about core developers?
> 
> I was mostly refering to ipatool's "push" command that I mostly used, but it's
> true there is also "start-review" or "am" commands that could be useful to
> others too.
> 
>> Shouldn't we make it easy to self-review own patches for everyone? Including
>> random people who want to submit few patches and go away? (Think how we can
>> apply usability principles to development.)
> 
> We should, I hope my reply did not suggest otherwise.

Hmm, so I finally tried ipatool and it blew up:

$ ../tool/ipatool --help
Configuration is specified in the file given by --config.
Run ipatool sample-config to see what needs to go in it.

$ ../tool/ipatool sample-config
Traceback (most recent call last):
  File "../tool/ipatool", line 701, in 
Context(docopt.docopt(__doc__)).run()
  File "../tool/ipatool", line 233, in __init__
with open(os.path.expanduser(options['--config'])) as conf_file:
FileNotFoundError: [Errno 2] No such file or directory:
'/home/pspacek/.ipa/toolconf.yaml'

... after a while I found out that it is too heavy Python magic inside (for me
as non-Pythonist) so I'm giving up.

Moreover it AFAIK does not work on tree with applied patches (which is normal
situation before you send the patched to list for review), so I'm not even
sure how I could integrate the review scripts into it.

For now I'm going to live with my review.sh scripts in following Git repo:
https://github.com/pspacek/ipareview.git

Pull requests or rewrites for ipatool are welcome.
You can take this as a Christmas present :-)

-- 
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 0374-0375, 395] Fix permissions on newly created directories

[Martin Basti]

On 14.12.2015 14:58, Tomas Babej wrote:


On 12/11/2015 07:19 PM, Martin Basti wrote:


On 10.12.2015 15:18, Martin Basti wrote:

Hello,

patch 0374 fixes the ticket, but I found more issues with directory
permission, I fixed them in 0375

https://fedorahosted.org/freeipa/ticket/5520

Patches attached.

Patches attached.


ACK, works as expected.

Pushed to master: 4272ba40ea909b1f783a6fada5b1eebb6efbdf93
I found issue, there was an extra mkdir instead of chmod, patch pushed 
under one-liner rule


Pushed to:
master: 403652b5b9cc5e80869019067cbe1424b5c02bd1
ipa-4-3: f3a8ef0601b21fe4ddace08d8fd2b7a821efd658
ipa-4-2: 066ecf466403e4e321253238d28a235c7bcc0cd6

From a6af381e996235ad8712ed6ba7995bb70176d24b Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Tue, 22 Dec 2015 16:34:32 +0100
Subject: [PATCH] Fix: replace mkdir with chmod

In original patches, extra mkdir has been added instead of chmod.

https://fedorahosted.org/freeipa/ticket/5520
---
 ipaplatform/base/services.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipaplatform/base/services.py b/ipaplatform/base/services.py
index 41b9654c90356fcc3819df0d68cbc2ec77178c4f..2ec84cdb21607cb51df6ad5fcd2ae515898bee44 100644
--- a/ipaplatform/base/services.py
+++ b/ipaplatform/base/services.py
@@ -420,7 +420,7 @@ class SystemdService(PlatformService):
 try:
 if not ipautil.dir_exists(srv_tgt):
 os.mkdir(srv_tgt)
-os.mkdir(srv_tgt, 0o755)
+os.chmod(srv_tgt, 0o755)
 if os.path.exists(srv_lnk):
 # Remove old link
 os.unlink(srv_lnk)
-- 
2.5.0

-- 
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] 4.3 on rawhide build task fail

[Petr Vobornik]

Build of 4.3 on Fedora rawhide failed at the end on rpmdiff check. 
Builds for all arches were successful and also works in COPR.

 0 free 1 open 4 done 0 failed
12284450 build (rawhide, 
/freeipa:b2442d51ba3f2a5f907f72e9bd90c5889bd89c0e): open 
(buildppcle-07.phx2.fedoraproject.org) -> FAILED: BuildError: mismatch 
when analyzing python3-ipatests-4.3.0-1.fc24.noarch.rpm, rpmdiff output was:

error: cannot open Packages index using db5 - Permission denied (13)
error: cannot open Packages database in /var/lib/rpm
error: cannot open Packages database in /var/lib/rpm
removed REQUIRES python3-ipalib(armv7hl-32) = 4.3.0-1.fc24
added REQUIRES python3-ipalib(x86-64) = 4.3.0-1.fc24
0 free 0 open 4 done 1 failed

Rebuild ended up with the same issue.

Do we have something wrong in spec file? Or the "Permission denied" is 
the cause and the issue might be in Fedora infra?


Any ideas?

Full output:

$ fedpkg build
Building freeipa-4.3.0-1.fc24 for rawhide
Created task: 12284450
Task info: http://koji.fedoraproject.org/koji/taskinfo?taskID=12284450
Watching tasks (this may be safely interrupted)...
12284450 build (rawhide, 
/freeipa:b2442d51ba3f2a5f907f72e9bd90c5889bd89c0e): free
12284450 build (rawhide, 
/freeipa:b2442d51ba3f2a5f907f72e9bd90c5889bd89c0e): free -> open 
(buildppcle-07.phx2.fedoraproject.org)
  12284454 buildSRPMFromSCM 
(/freeipa:b2442d51ba3f2a5f907f72e9bd90c5889bd89c0e): free
  12284454 buildSRPMFromSCM 
(/freeipa:b2442d51ba3f2a5f907f72e9bd90c5889bd89c0e): free -> open 
(buildhw-08.phx2.fedoraproject.org)
  12284454 buildSRPMFromSCM 
(/freeipa:b2442d51ba3f2a5f907f72e9bd90c5889bd89c0e): open 
(buildhw-08.phx2.fedoraproject.org) -> closed

  0 free  1 open  1 done  0 failed
  12284512 buildArch (freeipa-4.3.0-1.fc24.src.rpm, armv7hl): free
  12284514 buildArch (freeipa-4.3.0-1.fc24.src.rpm, i686): free
  12284513 buildArch (freeipa-4.3.0-1.fc24.src.rpm, x86_64): open 
(buildhw-05.phx2.fedoraproject.org)
  12284514 buildArch (freeipa-4.3.0-1.fc24.src.rpm, i686): free -> open 
(buildvm-26.phx2.fedoraproject.org)
  12284512 buildArch (freeipa-4.3.0-1.fc24.src.rpm, armv7hl): free -> 
open (arm04-builder19.arm.fedoraproject.org)
  12284514 buildArch (freeipa-4.3.0-1.fc24.src.rpm, i686): open 
(buildvm-26.phx2.fedoraproject.org) -> closed

  0 free  3 open  2 done  0 failed
  12284513 buildArch (freeipa-4.3.0-1.fc24.src.rpm, x86_64): open 
(buildhw-05.phx2.fedoraproject.org) -> closed

  0 free  2 open  3 done  0 failed
  12284512 buildArch (freeipa-4.3.0-1.fc24.src.rpm, armv7hl): open 
(arm04-builder19.arm.fedoraproject.org) -> closed

  0 free  1 open  4 done  0 failed
12284450 build (rawhide, 
/freeipa:b2442d51ba3f2a5f907f72e9bd90c5889bd89c0e): open 
(buildppcle-07.phx2.fedoraproject.org) -> FAILED: BuildError: mismatch 
when analyzing python3-ipatests-4.3.0-1.fc24.noarch.rpm, rpmdiff output was:

error: cannot open Packages index using db5 - Permission denied (13)
error: cannot open Packages database in /var/lib/rpm
error: cannot open Packages database in /var/lib/rpm
removed REQUIRES python3-ipalib(armv7hl-32) = 4.3.0-1.fc24
added   REQUIRES python3-ipalib(x86-64) = 4.3.0-1.fc24
  0 free  0 open  4 done  1 failed

12284450 build (rawhide, 
/freeipa:b2442d51ba3f2a5f907f72e9bd90c5889bd89c0e) failed


--
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 0077-8] ipa-replica-prepare: Add '--auto-reverse' and, '--allow-zone-overlap' options

[Petr Spacek]

On 22.12.2015 15:14, David Kupka wrote:
> https://fedorahosted.org/freeipa/ticket/5563

NACK:

$ ipa-replica-prepare vm-058-106.abc.idm.lab.eng.brq.redhat.com
--ip-address=10.34.58.106 --reverse-zone=58.34.10.in-addr.arpa.
Directory Manager (existing master) password:

Checking DNS domain 58.34.10.in-addr.arpa., please wait ...
Values instance has no attribute 'unattended'
The ipa-replica-prepare command failed.

-- 
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 0077] ipa-dns-install: Do not check for zone overlap when DNS, installed.

[Martin Basti]

On 22.12.2015 14:33, Petr Spacek wrote:

On 22.12.2015 13:14, David Kupka wrote:

On 18/12/15 15:39, Petr Vobornik wrote:

On 12/18/2015 12:04 PM, Petr Vobornik wrote:

On 12/18/2015 11:26 AM, David Kupka wrote:

Standalone DNS installer always performed overlap check effectively
preventing installation on replica when other DNS instance was already
installed in topology.



I don't like the position of api argument in the install_check. It is
not consistent with install_check in KRA plus it's between two related
options: standalone and replica.

Is there a reason behind it which I don't see?

NACK, the API call is incorrect.

It should be api.Command['dns_is_enabled']()['result'] or
api.Command.dns_is_enabled()['result']


ipa-dns-install --forwarder=$DNS_FORWARDER

The log file for this installation can be found in
/var/log/ipaserver-install.log
Unexpected error - see /var/log/ipaserver-install.log for details:
TypeError: __call__() takes exactly 1 argument (2 given)


log file:

2015-12-18T14:33:30Z DEBUG   File
"/usr/lib/python2.7/site-packages/ipaserver/install/installutils.py",
line 736, in run_script
  return_value = main_function()

File "/sbin/ipa-dns-install", line 137, in main
  dns_installer.install_check(True, api, False, options,
hostname=api.env.host)

File "/usr/lib/python2.7/site-packages/ipaserver/install/dns.py",
line 113, in install_check
  already_enabled = api.Command('dns_is_enabled')['result']

2015-12-18T14:33:30Z DEBUG The ipa-dns-install command failed,
exception: TypeError: __call__() takes exactly 1 argument (2 given)


Thanks for catch. Sometimes I mess up with patch files. Fixed patch attached.

ACK, works for me.


Pushed to master: 8ad39a974fd4e1cc6fd2a09b1d9d0c8724983b15
Pushed to ipa-4-3: bfad829586f40e20ada21ea932377089901aaf61

--
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] Add option to disable setkeytab extended operations

[Petr Spacek]

On 1.12.2015 12:00, Alexander Bokovoy wrote:
> On Tue, 01 Dec 2015, Alexander Bokovoy wrote:
>> On Tue, 01 Dec 2015, Petr Spacek wrote:
>>> On 1.12.2015 09:47, Alexander Bokovoy wrote:
 On Tue, 01 Dec 2015, Petr Spacek wrote:
> On 1.12.2015 09:21, Alexander Bokovoy wrote:
>> On Tue, 01 Dec 2015, Petr Spacek wrote:
>>> On 24.11.2015 20:42, Simo Sorce wrote:
 Since some time we use the getkeytab operation to fetch keytabs on 
 newer
 clients. According to bug #232 setkeytab can be used to circumvent
 password quality controls so it needs to be slowly retired.

 The attached patches implement #5485 in 2 parts.

 The first introduces the option DisableSetKeytab which globally 
 disables
 the setkeytab extended operation. This is set to false by default for
 backwards compatibility.

 The second introduces an option called DisableUserSetKeytab, which is
 active by default in new installs (but not in upgraded ones), and only
 disables the use of setkeytab for ipa suers, but not for 
 hosts/services.
 This is because user's are the ones that may abuse the interface to
 escape password policies and users also normally do not acquire 
 keytabs,
 so it is a safe bet to disable just them by default in new installs.
>>>
>>> On a related note, how this works with plain kadmin & kpasswd protocols?
>> It is unrelated. We don't support principal manipulation via kadmin
>> protocol.
>
> Sure, I know that, but I'm trying to find out if we re-invented the wheel 
> or
> if our wheel has some additional features which cannot be incorporated to
> the
> original wheel :-)
 There is no need to incorporate something specific into kadmin protocol,
 the problem is with the fact that we don't have access controls within
 our KDC driver. It always connects to LDAP server over ldapi as root and
 thus maps to cn=Directory Manager which bypasses LDAP ACIs. As such,
 this means we'll need to have some access control added to KDC DAL
 driver before we can allow kadmin operations on principals.

 Adding those access controls is not an easy feat.
>>>
>>> Hmm, why is that? We could use LDAP Proxy control (RFC 4370) and let DS to
>>> evaluate ACIs as usual. The change in kadmin would be adding LDAP Proxy
>>> control with proper DN (i.e. mapping from principal name to DN) to the LDAP
>>> requests.
>> Not sure how does it help:
>> $ ldapsearch -Dcn=Directory\ Manager -W -e
>> \!authzid=dn:uid=abokovoy,cn=users,cn=accounts,dc=vda,dc=li -b
>> uid=admin,cn=users,cn=accounts,dc=vda,dc=li userPassword
>> Enter LDAP Password: # extended LDIF
>> #
>> # LDAPv3
>> # base  with scope subtree
>> # filter: (objectclass=*)
>> # requesting: userPassword #
>>
>> # admin, users, accounts, vda.li
>> dn: uid=admin,cn=users,cn=accounts,dc=vda,dc=li
>> userPassword:: =
>> =
>>
>> # search result
>> search: 2
>> result: 0 Success
>>
>> # numResponses: 2
>> # numEntries: 1
>> $ ldapsearch -Y GSSAPI -e
>> \!authzid=dn:uid=admin,cn=users,cn=accounts,dc=vda,dc=li -b
>> uid=admin,cn=users,cn=accounts,dc=vda,dc=li userPassword
>> SASL/GSSAPI authentication started
>> SASL username: aboko...@vda.li
>> SASL SSF: 56
>> SASL data security layer installed.
>> # extended LDIF
>> #
>> # LDAPv3
>> # base  with scope subtree
>> # filter: (objectclass=*)
>> # requesting: userPassword #
>>
>> # search result
>> search: 4
>> result: 0 Success
>>
>> # numResponses: 1
>>
>> As you can see, posing different authzid does not allow to get through
>> ACLs unless you were already a directory manager. Which means the
>> proxyauth control is not really useful here:
>>
>> [01/Dec/2015:10:51:22 +0100] conn=12466 op=0 BIND dn="cn=Directory Manager"
>> method=128 version=3
>> [01/Dec/2015:10:51:22 +0100] conn=12466 op=0 RESULT err=0 tag=97 nentries=0
>> etime=0 dn="cn=directory manager"
>> [01/Dec/2015:10:51:22 +0100] conn=12466 op=1 SRCH
>> base="uid=admin,cn=users,cn=accounts,dc=vda,dc=li" scope=2
>> filter="(objectClass=*)" attrs="userPassword"
>> authzid="uid=abokovoy,cn=users,cn=accounts,dc=vda,dc=li"
>> [01/Dec/2015:10:51:22 +0100] conn=12466 op=1 RESULT err=0 tag=101 nentries=1
>> etime=0 notes=U
>> [01/Dec/2015:10:51:22 +0100] conn=12466 op=2 UNBIND
>>
>> and
>>
>> [01/Dec/2015:10:48:44 +0100] conn=12465 op=0 BIND dn="" method=sasl
>> version=3 mech=GSSAPI
>> [01/Dec/2015:10:48:44 +0100] conn=12465 op=0 RESULT err=14 tag=97 nentries=0
>> etime=0, SASL bind in progress
>> [01/Dec/2015:10:48:44 +0100] conn=12465 op=1 BIND dn="" method=sasl
>> version=3 mech=GSSAPI
>> [01/Dec/2015:10:48:44 +0100] conn=12465 op=1 RESULT err=14 tag=97 nentries=0
>> etime=0, SASL bind in progress
>> [01/Dec/2015:10:48:44 +0100] conn=12465 op=2 BIND dn="" method=sasl
>> version=3 mech=GSSAPI
>> [01/Dec/2015:10:48:44 +

Re: [Freeipa-devel] ipa-devel repos on jdennis.fedorapeople.org

[Petr Spacek]

On 27.8.2015 10:34, Alexander Bokovoy wrote:
> On Thu, 27 Aug 2015, Petr Spacek wrote:
>> On 15.7.2015 09:44, Jan Pazdziora wrote:
>>> On Tue, Jul 14, 2015 at 12:49:23PM -0400, John Dennis wrote:
 On 07/14/2015 12:03 PM, Petr Spacek wrote:
> Hello,
>
> Is anyone using repos
> https://jdennis.fedorapeople.org/ipa-devel/
> ?
>
> AFAIK nobody in Brno is seriously using it but I'm not sure about people
> outside the Brno.
>
> Could we use COPR instead and get out of builder business? Upcoming lab
> maintenance window could be a good time to do that.

 I would love to get out of the builder business and I suspect Nalin would 
 as
 well [1]. The question came up in our Monday meeting as well. Nobody seem 
 to
 know if anyone was using these builds and why we weren't using COPR. The
>>>
>>> The Fedora infra admins should be able to provide HTTP logs for the
>>> repo, if you needs some numbers about potential usage.
>>
>> That is a good idea! I got logs from Fedora admins and as far as I can tell,
>> in the last month there were only 7 RPM downloads and nothing else.
>>
>> The 7 hits I found was for
>> /ipa-devel/rhel/6/x86_64/os/sssd-1.13.1-0.20150813T1121Zgit137d5dd.el6.i686.rpm
>> and
>> other packages from the same version.
>>
>> I did not find any hits for IPA packages at all. All the remaining traffic
>> (except the 7 RPM hits) was from repo data refreshes:
>> - 83 % is RHEL 5 repodata
>> - 13 % is RHEL 6 repodata
>> - remaining ~ 4 % of noise is Fedora repodata
>>
>> It seems to me that we can get out the builder business completely and
>> decommission ipa-devel and replace it with COPR.
>>
>> Do you agree? John? Nathaniel? Stephen? Anyone? :-)
> Yes, I think we can decommission this repository thanks to COPR
> infrastructure.

John, the machines which used to generate the repos are basically dead now.

Could you remove the directories and replace them with a README with sentence
that the repos were discontinued, please?

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

[Freeipa-devel] [PATCH 0077-8] ipa-replica-prepare: Add '--auto-reverse' and, '--allow-zone-overlap' options

[David Kupka]

https://fedorahosted.org/freeipa/ticket/5563
--
David Kupka
From 7d8bf4809059f97ff62f4b9a73a486ed7d19d6bb Mon Sep 17 00:00:00 2001
From: David Kupka 
Date: Tue, 22 Dec 2015 13:53:41 +
Subject: [PATCH] ipa-replica-prepare: Add '--auto-reverse' and
 '--allow-zone-overlap' options

Opiton should be added to ipa-replica-prepare when it was added to
ipa-{server,replica,dns}-install but was forgotten.

https://fedorahosted.org/freeipa/ticket/5563
---
 ipaserver/install/ipa_replica_prepare.py | 5 +
 1 file changed, 5 insertions(+)

diff --git a/ipaserver/install/ipa_replica_prepare.py b/ipaserver/install/ipa_replica_prepare.py
index cef0228..e9fd2d9 100644
--- a/ipaserver/install/ipa_replica_prepare.py
+++ b/ipaserver/install/ipa_replica_prepare.py
@@ -80,6 +80,11 @@ class ReplicaPrepare(admintool.AdminTool):
 parser.add_option("--no-reverse", dest="no_reverse",
 action="store_true", default=False,
 help="do not create reverse DNS zone")
+parser.add_option("--auto-reverse", dest="auto_reverse", default=False,
+action="store_true", help="create necessary DNS zones")
+parser.add_option("--allow-zone-overlap", dest="allow_zone_overlap",
+action="store_true", default=False, help="create DNS "
+"zone even if it already exists")
 parser.add_option("--no-pkinit", dest="setup_pkinit",
 action="store_false", default=True,
 help="disables pkinit setup steps")
-- 
2.5.0

From 95657910e6918e3134a20244ce3def28150ced8e Mon Sep 17 00:00:00 2001
From: David Kupka 
Date: Tue, 22 Dec 2015 14:05:57 +
Subject: [PATCH] installer: Change reverse zones question to better reflect
 reality.

https://fedorahosted.org/freeipa/ticket/5563
---
 ipaserver/install/bindinstance.py | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/ipaserver/install/bindinstance.py b/ipaserver/install/bindinstance.py
index 8daca55..3574699 100644
--- a/ipaserver/install/bindinstance.py
+++ b/ipaserver/install/bindinstance.py
@@ -73,7 +73,10 @@ named_conf_include_template = "include \"%(path)s\";\n"
 
 
 def create_reverse():
-return ipautil.user_input("Do you want to configure the reverse zone?", True)
+return ipautil.user_input(
+"Do you want to search for missing reverse zones?",
+True
+)
 
 def named_conf_exists():
 try:
-- 
2.5.0

-- 
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] 942 webui: add examples to network address validator error message

[Gabe Alford]

LGTM.

Gabe

On Tue, Dec 22, 2015 at 6:06 AM, Petr Vobornik  wrote:

> https://fedorahosted.org/freeipa/ticket/5532
> --
> 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
>
-- 
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 0072-0081] DNSSEC: fixes

[Petr Spacek]

On 21.12.2015 18:56, Martin Basti wrote:
> 
> 
> On 21.12.2015 15:45, Martin Basti wrote:
>>
>>
>> On 21.12.2015 15:33, Petr Spacek wrote:
>>> Hello,
>>>
>>> this patch set fixes key rotation in DNSSEC.
>>>
>>> You can use attached template files for OpenDNSSEC config to shorten time
>>> intervals between key rotations.
>>>
>>> Please let me know if you have any questions, I'm all ears!
>>>
>> Please fix whitespace error:
>>
>> Applying: DNSSEC: logging improvements in ldapkeydb.py
>> /home/mbasti/work/freeipa-devel/.git/rebase-apply/patch:14: trailing
>> whitespace.
>>
>> warning: 1 line adds whitespace errors.
>>
> *) DNSSEC: ipa-dnskeysyncd: call ods-signer ldap-cleanup on zone removal
> 
> Is is safe to do not use try - except with ipatuil.run()? What if ods-signer
> command failed?

That is intentional. The call should never fail, so if it fails there is no
way how to recover cleanly except restarting the daemon.

The unhandled exception will kill the daemon and systemd will restart it later 
on.


> *) DNSSEC: Improve error reporting from ipa-ods-exporter
> IMO log.exception(ex)  is enough, do we need to add traceback to msg?

msg is sent over socket to another process (see send_systemd_reply(conn, msg)
call in finally: block). Without this the remote party would not receive the
error information.


> *) DNSSEC: Make sure that current state in OpenDNSSEC matches key state in 
> LDAP
> I think this is okay because we want to use KSK instantly, but just to be
> sure, is Publish->Activate okay?
> +bind_times['idnsSecKeyActivate'] = ods_times['idnsSecKeyPublish']
> 
> Just to be sure how this will be handle during KSK key rotation?

We have to copy semantics from OpenDNSSEC. Please see design page
https://fedorahosted.org/bind-dyndb-ldap/wiki/BIND9/Design/DNSSEC/OpenDNSSEC2BINDKeyStates
, it describes in detail why I done it this way.


> *) DNSSEC: Make sure that current key state in LDAP matches key state in BIND
> LGTM
> 
> *) DNSSEC: remove obsolete TODO note
> ACK
> 
> *) DNSSEC: add debug mode to ldapkeydb.py
> A)
> You can remove __str__ method, python will use __repr__ as default

Done.


> B)
> for attr in ['ipaPrivateKey', 'ipaPublicKey', 'ipk11publickeyinfo']:
> Do we need to sanitize *public*Key and publicKeyinfo?

Yes, we need it. The output with any of ['ipaPrivateKey', 'ipaPublicKey',
'ipk11publickeyinfo'] is huge blob and printing it does not help readability.
Purpose of the patch is to make it easy to read and debug so printing useless
blobs would go directly against the purpose :-)


> C)
> in odsmgr.py is used ipa_log_manager, can we use the same for consistency?

Fixed, thanks.


> D)
> Do we need logging there, everything is printed via print except debug info
> about connecting, can you just redirect it to stderr, and usable data leave in
> stdout?

Yes, we need it because it eases debugging. print() prints useful information
to stdout. 'Garbage' about connecting to LDAP, IPA framework initialization
and so on does via logger to stderr, so it can be easily separated from useful
information using redirection in BASH.

I've added a comment right below if __name__ == '__main__': to make it clear
why we do not use logger in there.


> *) DNSSEC: logging improvements in ldapkeydb.py
> IMO commit message should be: " in ipa-ods-exporter"
> 
> Otherwise LGTM

Fixed, thanks.


> *) DNSSEC: remove keys purged by OpenDNSSEC from master HSM from LDAP
> 
> A) coding style: please use (), instead of "\"
> assert set(pubkeys_local) == set(privkeys_local), (
> "IDs of private and public keys for DNS zones in local HSM does "
> "not match to key pairs: %s vs. %s" %
> (hex_set(pubkeys_local), hex_set(privkeys_local))
> )

Fixed.


> B) coding style
> assert not matched_already, (
> "key %s is in more than one keyset" % hexlify(keyid)
> )

Not relevant anymore, see below.


> C) schedule_key_deletion()
> how about case when keyid is not in any keyset, then keyid will not be
> replaced by object and it blow up somewhere else

Not relevant anymore, see below.


> D) +class KeyDeleter(object):
> I would like to have a check there which blows up nicely if _update_key() is
> called twice on the same object. With current implementation you will get
> NoneType has no delete_entry method.

Not relevant anymore, see below.


> E)
> I somehow does not like the placeholder object.  Could we just extend Key
> object with attribute "to_be_deleted" or something similar, and if this
> attribute is set to True, Key._update_key() can remove, instead of creation a
> new object.
> Key.prepare_deletion() can set the value "to_be_deleted" to True.

Main purpose of the KeyDeleter object was to be incompatible the Key object. I
want to be 100 % that is not possible to call schedule_delete() and
subsequenty modify the Key object.

I've reworked the Key object so it has schedule_deletion() method and that all
other

Re: [Freeipa-devel] [PATCH 0077] ipa-dns-install: Do not check for zone overlap when DNS, installed.

[Petr Spacek]

On 22.12.2015 13:14, David Kupka wrote:
> On 18/12/15 15:39, Petr Vobornik wrote:
>> On 12/18/2015 12:04 PM, Petr Vobornik wrote:
>>> On 12/18/2015 11:26 AM, David Kupka wrote:
 Standalone DNS installer always performed overlap check effectively
 preventing installation on replica when other DNS instance was already
 installed in topology.


>>>
>>> I don't like the position of api argument in the install_check. It is
>>> not consistent with install_check in KRA plus it's between two related
>>> options: standalone and replica.
>>>
>>> Is there a reason behind it which I don't see?
>>
>> NACK, the API call is incorrect.
>>
>> It should be api.Command['dns_is_enabled']()['result'] or
>> api.Command.dns_is_enabled()['result']
>>
>>
>> ipa-dns-install --forwarder=$DNS_FORWARDER
>>
>> The log file for this installation can be found in
>> /var/log/ipaserver-install.log
>> Unexpected error - see /var/log/ipaserver-install.log for details:
>> TypeError: __call__() takes exactly 1 argument (2 given)
>>
>>
>> log file:
>>
>> 2015-12-18T14:33:30Z DEBUG   File
>> "/usr/lib/python2.7/site-packages/ipaserver/install/installutils.py",
>> line 736, in run_script
>>  return_value = main_function()
>>
>>File "/sbin/ipa-dns-install", line 137, in main
>>  dns_installer.install_check(True, api, False, options,
>> hostname=api.env.host)
>>
>>File "/usr/lib/python2.7/site-packages/ipaserver/install/dns.py",
>> line 113, in install_check
>>  already_enabled = api.Command('dns_is_enabled')['result']
>>
>> 2015-12-18T14:33:30Z DEBUG The ipa-dns-install command failed,
>> exception: TypeError: __call__() takes exactly 1 argument (2 given)
>>
> Thanks for catch. Sometimes I mess up with patch files. Fixed patch attached.

ACK, works for me.

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

[Freeipa-devel] [PATCH] 943 webui: pwpolicy cospriority field was marked as required

[Petr Vobornik]

https://fedorahosted.org/freeipa/ticket/5553
--
Petr Vobornik
From 18c116d50b72749a439334b467d017486a77e56a Mon Sep 17 00:00:00 2001
From: Petr Vobornik 
Date: Tue, 22 Dec 2015 14:22:21 +0100
Subject: [PATCH] webui: pwpolicy cospriority field was marked as required

https://fedorahosted.org/freeipa/ticket/5553
---
 install/ui/src/freeipa/policy.js | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/install/ui/src/freeipa/policy.js b/install/ui/src/freeipa/policy.js
index 2766c9f5e6b0305cfc4dbb8a2b0e87c8005d6e84..6c20cc14677fc39f8da349d43ce717cb5d4f2d42 100644
--- a/install/ui/src/freeipa/policy.js
+++ b/install/ui/src/freeipa/policy.js
@@ -84,7 +84,10 @@ return {
 other_field: 'cn',
 required: true
 },
-'cospriority'
+{
+name: 'cospriority',
+required: true
+}
 ],
 height: 300
 }
-- 
2.4.3

-- 
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 0373] Upgrade: Fix IPA version comparison

[Martin Basti]

On 22.12.2015 14:10, Martin Basti wrote:



On 22.12.2015 14:04, Petr Spacek wrote:

On 14.12.2015 14:29, Tomas Babej wrote:


On 12/14/2015 10:21 AM, Martin Basti wrote:


On 14.12.2015 09:24, Martin Kosek wrote:

On 12/14/2015 07:21 AM, Jan Cholasta wrote:

On 11.12.2015 19:01, Tomas Babej wrote:

On 12/11/2015 09:36 AM, Martin Kosek wrote:

On 12/10/2015 05:09 PM, Martin Basti wrote:

On 10.12.2015 15:49, Tomas Babej wrote:

On 12/10/2015 11:23 AM, Martin Basti wrote:

On 10.12.2015 09:13, Lukas Slebodnik wrote:

On (09/12/15 19:22), Martin Basti wrote:

https://fedorahosted.org/freeipa/ticket/5535

Patch attached.
>From 8ef93485d61e8732166fb0c5b6c4559209740f3e Mon Sep 17 
00:00:00

2001

From: Martin Basti 
Date: Wed, 9 Dec 2015 18:53:35 +0100
Subject: [PATCH] Fix version comparison

Use RPM library to compare vendor versions of IPA for redhat
platform

https://fedorahosted.org/freeipa/ticket/5535
---
freeipa.spec.in |  2 ++
ipaplatform/redhat/tasks.py | 19 +++
2 files changed, 21 insertions(+)

diff --git a/freeipa.spec.in b/freeipa.spec.in
index
9f82b3695fb10c4db65cc31278364b3b34e26098..09feba7b8324f5e645da3e8010de86b6c3ee5ab9 






100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -166,6 +166,8 @@ Requires: %{etc_systemd_dir}
Requires: gzip
Requires: python-gssapi >= 1.1.0
Requires: custodia
+Requires: rpm-python
+Requires: rpmdevtools

Could you explain why do you need the 2nd package?
It does not contains any python modules
and I cannot see usage of any binary in this patch

LS

Thanks for this catch, it is actually located in yum package, I
rather
copy stringToVersion function from there to IPA, to avoid
dependency
hell

Updated patch attached.



Looking good. The __cmp__ function, however, is not available in
Python
3. As we will eventually support python3 in RHEL as well, 
maybe we

should make sure even platform-dependent parts are python3
compatible?
For the future's sake.

Tomas


Thanks,

python 3 compatible patch attached.

I wonder why we have to add so much code here and reimplement RPM
version checking if it is already provided by rpmdevtools:

~~~
$ /usr/bin/rpmdev-vercmp ipa-4.2.0-15.el7 4.2.0-15.el7_2.3; 
echo $?

WARNING: hyphen in release1: 4.2.0-15.el7

rpmdev-vercmp  


rpmdev-vercmp  
rpmdev-vercmp # with no arguments, prompt

Exit status is 0 if the EVR's are equal, 11 if EVR1 is newer, and
12 if
EVR2
is newer.  Other exit statuses indicate problems.

ipa-4.2.0-15.el7 < 4.2.0-15.el7_2.3
12
~~~

which could be directly called from __eq__ or __lt__, since we 
are in

platform specific code anyway already.

Martin
Imho we should generally prefer reaching out to a Python library 
rather

launching a subprocess to compare the versions, it's unnecessary
overhead.

I would not be too worried about miliseconds longer execution on a
function
called during RPM upgrade.


That said, the main argument for the usage of rpm-python over
rpmdevtools I see is that rpm-python is very likely to be 
present on

the
system (i.e. it is yum's own dependency), while rpmdevtools will 
not be

present by default.

   From the standpoint of trying to minimize the size of freeipa
installation (i.e recent wget -> curl migration), we should keep 
the

spirit here and do not introduce a dependency for a collection of
developer tools.

/2 cents

+1, also a single function is hardly "much code".
Ok then. If you all want to add yet another N-V-R parsing function 
in the

FreeIPA code, I can live with it (with raised eyebrows though).

Rebased patch attached.

I tested the patch, and it works fine - so conditional ACK from me for
the current iteration of the patch, given developer consensus which was
not reached yet.

There's a split of opinions (external binary camp vs. copy&paste camp),
so we need to decide if we both camps are OK with proceeding.
Further inspection shows that rpmdevtools depends on Perl stack which 
seems to
be too much for such a simple thing. So, I'm hesitantly changing my 
NACK to

ACK for copy&paste camp.


It seems that copy&paste camp won.

Pushed to:
master: 91913c5ba7c380fe6456e1c3e35fcbfbecef5ff1
ipa-4-3: a249de3b00f20956214a6ee0c1d614b972826637


Patch for ipa-4-2 needs rebase ... wait for it please

Martin^2


Pushed to ipa-4-2: dccdade484698d5ef553f03ce6aab5d9db7a7cf0

--
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 0373] Upgrade: Fix IPA version comparison

[Martin Basti]

On 22.12.2015 14:04, Petr Spacek wrote:

On 14.12.2015 14:29, Tomas Babej wrote:


On 12/14/2015 10:21 AM, Martin Basti wrote:


On 14.12.2015 09:24, Martin Kosek wrote:

On 12/14/2015 07:21 AM, Jan Cholasta wrote:

On 11.12.2015 19:01, Tomas Babej wrote:

On 12/11/2015 09:36 AM, Martin Kosek wrote:

On 12/10/2015 05:09 PM, Martin Basti wrote:

On 10.12.2015 15:49, Tomas Babej wrote:

On 12/10/2015 11:23 AM, Martin Basti wrote:

On 10.12.2015 09:13, Lukas Slebodnik wrote:

On (09/12/15 19:22), Martin Basti wrote:

https://fedorahosted.org/freeipa/ticket/5535

Patch attached.

>From 8ef93485d61e8732166fb0c5b6c4559209740f3e Mon Sep 17 00:00:00
2001

From: Martin Basti 
Date: Wed, 9 Dec 2015 18:53:35 +0100
Subject: [PATCH] Fix version comparison

Use RPM library to compare vendor versions of IPA for redhat
platform

https://fedorahosted.org/freeipa/ticket/5535
---
freeipa.spec.in |  2 ++
ipaplatform/redhat/tasks.py | 19 +++
2 files changed, 21 insertions(+)

diff --git a/freeipa.spec.in b/freeipa.spec.in
index
9f82b3695fb10c4db65cc31278364b3b34e26098..09feba7b8324f5e645da3e8010de86b6c3ee5ab9




100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -166,6 +166,8 @@ Requires: %{etc_systemd_dir}
Requires: gzip
Requires: python-gssapi >= 1.1.0
Requires: custodia
+Requires: rpm-python
+Requires: rpmdevtools

Could you explain why do you need the 2nd package?
It does not contains any python modules
and I cannot see usage of any binary in this patch

LS

Thanks for this catch, it is actually located in yum package, I
rather
copy stringToVersion function from there to IPA, to avoid
dependency
hell

Updated patch attached.



Looking good. The __cmp__ function, however, is not available in
Python
3. As we will eventually support python3 in RHEL as well, maybe we
should make sure even platform-dependent parts are python3
compatible?
For the future's sake.

Tomas


Thanks,

python 3 compatible patch attached.

I wonder why we have to add so much code here and reimplement RPM
version checking if it is already provided by rpmdevtools:

~~~
$ /usr/bin/rpmdev-vercmp ipa-4.2.0-15.el7 4.2.0-15.el7_2.3; echo $?
WARNING: hyphen in release1: 4.2.0-15.el7

rpmdev-vercmp  
rpmdev-vercmp  
rpmdev-vercmp # with no arguments, prompt

Exit status is 0 if the EVR's are equal, 11 if EVR1 is newer, and
12 if
EVR2
is newer.  Other exit statuses indicate problems.

ipa-4.2.0-15.el7 < 4.2.0-15.el7_2.3
12
~~~

which could be directly called from __eq__ or __lt__, since we are in
platform specific code anyway already.

Martin

Imho we should generally prefer reaching out to a Python library rather
launching a subprocess to compare the versions, it's unnecessary
overhead.

I would not be too worried about miliseconds longer execution on a
function
called during RPM upgrade.


That said, the main argument for the usage of rpm-python over
rpmdevtools I see is that rpm-python is very likely to be present on
the
system (i.e. it is yum's own dependency), while rpmdevtools will not be
present by default.

   From the standpoint of trying to minimize the size of freeipa
installation (i.e recent wget -> curl migration), we should keep the
spirit here and do not introduce a dependency for a collection of
developer tools.

/2 cents

+1, also a single function is hardly "much code".

Ok then. If you all want to add yet another N-V-R parsing function in the
FreeIPA code, I can live with it (with raised eyebrows though).

Rebased patch attached.

I tested the patch, and it works fine - so conditional ACK from me for
the current iteration of the patch, given developer consensus which was
not reached yet.

There's a split of opinions (external binary camp vs. copy&paste camp),
so we need to decide if we both camps are OK with proceeding.

Further inspection shows that rpmdevtools depends on Perl stack which seems to
be too much for such a simple thing. So, I'm hesitantly changing my NACK to
ACK for copy&paste camp.


It seems that copy&paste camp won.

Pushed to:
master: 91913c5ba7c380fe6456e1c3e35fcbfbecef5ff1
ipa-4-3: a249de3b00f20956214a6ee0c1d614b972826637


Patch for ipa-4-2 needs rebase ... wait for it please

Martin^2

--
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 0365] Remove unused KRA code from ipa-server-install

[Martin Basti]

On 15.12.2015 16:43, David Kupka wrote:

On 15/12/15 16:20, Martin Kosek wrote:

On 12/15/2015 07:42 AM, Jan Cholasta wrote:

On 14.12.2015 16:54, Alexander Bokovoy wrote:

On Mon, 14 Dec 2015, David Kupka wrote:

...
We can always have both. With the new installer framework it is 
trivial to fold
installers like this without code duplication. It is still work in 
progress
though, so I would prefer not to add --setup-kra to 
ipa-server-install now (if

ever).


+1, I would also rather not make ipa-server-install too monolithic. 
Even from
testing POV, I think doing KRA fixes in one installer is much more 
easier than
doing it also in replica or server installers (although Jan's patches 
help

here, that's true).



If you look on the patch Martin send, you will see that there is 
nothing monolithic in installing KRA in ipa-server-install.
In fact, it calls kra.check_install() and kra.install(), the very same 
functions that are called from ipa-kra-install.


But it seems that majority of us is decided to remove the ability to 
install KRA from ipa-server-install and since I've tested the patch 
and didn't find any issues we can push it, ACK.



Pushed to master: e622da3e1a25c05b77fed538634c284e68e2397f

--
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] 942 webui: add examples to network address validator error message

[Petr Vobornik]

https://fedorahosted.org/freeipa/ticket/5532
--
Petr Vobornik
From 8ba63e0e76c75253fe4cd16d4aae245a1714ea77 Mon Sep 17 00:00:00 2001
From: Petr Vobornik 
Date: Tue, 22 Dec 2015 14:05:02 +0100
Subject: [PATCH] webui: add examples to network address validator error
 message

https://fedorahosted.org/freeipa/ticket/5532
---
 install/ui/test/data/ipa_init.json | 2 +-
 ipalib/plugins/internal.py | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/install/ui/test/data/ipa_init.json b/install/ui/test/data/ipa_init.json
index 3ac827811c347bc03e3c93ea1d91a5f9cf558080..310eef1055a19dd40f8221c2967b09773595b80b 100644
--- a/install/ui/test/data/ipa_init.json
+++ b/install/ui/test/data/ipa_init.json
@@ -696,7 +696,7 @@
 "ip_v6_address": "Not a valid IPv6 address",
 "max_value": "Maximum value is ${value}",
 "min_value": "Minimum value is ${value}",
-"net_address": "Not a valid network address",
+"net_address": "Not a valid network address (examples: 2001:db8::/64, 192.0.2.0/24)",
 "parse": "Parse error",
 "port": "'${port}' is not a valid port",
 "required": "Required field",
diff --git a/ipalib/plugins/internal.py b/ipalib/plugins/internal.py
index 1c58b6bd1dc5bc97398374f1c32ff30409e5a001..2cead97926ccb8c5c33fe69f79ab3a479bdf1c53 100644
--- a/ipalib/plugins/internal.py
+++ b/ipalib/plugins/internal.py
@@ -843,7 +843,7 @@ class i18n_messages(Command):
 "ip_v6_address": _('Not a valid IPv6 address'),
 "max_value": _("Maximum value is ${value}"),
 "min_value": _("Minimum value is ${value}"),
-"net_address": _("Not a valid network address"),
+"net_address": _("Not a valid network address (examples: 2001:db8::/64, 192.0.2.0/24)"),
 "parse": _("Parse error"),
 "port": _("'${port}' is not a valid port"),
 "required": _("Required field"),
-- 
2.4.3

-- 
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 0373] Upgrade: Fix IPA version comparison

[Petr Spacek]

On 14.12.2015 14:29, Tomas Babej wrote:
> 
> 
> On 12/14/2015 10:21 AM, Martin Basti wrote:
>>
>>
>> On 14.12.2015 09:24, Martin Kosek wrote:
>>> On 12/14/2015 07:21 AM, Jan Cholasta wrote:
 On 11.12.2015 19:01, Tomas Babej wrote:
>
> On 12/11/2015 09:36 AM, Martin Kosek wrote:
>> On 12/10/2015 05:09 PM, Martin Basti wrote:
>>>
>>> On 10.12.2015 15:49, Tomas Babej wrote:
 On 12/10/2015 11:23 AM, Martin Basti wrote:
> On 10.12.2015 09:13, Lukas Slebodnik wrote:
>> On (09/12/15 19:22), Martin Basti wrote:
>>> https://fedorahosted.org/freeipa/ticket/5535
>>>
>>> Patch attached.
>> >From 8ef93485d61e8732166fb0c5b6c4559209740f3e Mon Sep 17 00:00:00
>> 2001
>>> From: Martin Basti 
>>> Date: Wed, 9 Dec 2015 18:53:35 +0100
>>> Subject: [PATCH] Fix version comparison
>>>
>>> Use RPM library to compare vendor versions of IPA for redhat
>>> platform
>>>
>>> https://fedorahosted.org/freeipa/ticket/5535
>>> ---
>>> freeipa.spec.in |  2 ++
>>> ipaplatform/redhat/tasks.py | 19 +++
>>> 2 files changed, 21 insertions(+)
>>>
>>> diff --git a/freeipa.spec.in b/freeipa.spec.in
>>> index
>>> 9f82b3695fb10c4db65cc31278364b3b34e26098..09feba7b8324f5e645da3e8010de86b6c3ee5ab9
>>>
>>>
>>>
>>>
>>> 100644
>>> --- a/freeipa.spec.in
>>> +++ b/freeipa.spec.in
>>> @@ -166,6 +166,8 @@ Requires: %{etc_systemd_dir}
>>> Requires: gzip
>>> Requires: python-gssapi >= 1.1.0
>>> Requires: custodia
>>> +Requires: rpm-python
>>> +Requires: rpmdevtools
>> Could you explain why do you need the 2nd package?
>> It does not contains any python modules
>> and I cannot see usage of any binary in this patch
>>
>> LS
> Thanks for this catch, it is actually located in yum package, I
> rather
> copy stringToVersion function from there to IPA, to avoid
> dependency
> hell
>
> Updated patch attached.
>
>
 Looking good. The __cmp__ function, however, is not available in
 Python
 3. As we will eventually support python3 in RHEL as well, maybe we
 should make sure even platform-dependent parts are python3
 compatible?
 For the future's sake.

 Tomas

>>> Thanks,
>>>
>>> python 3 compatible patch attached.
>> I wonder why we have to add so much code here and reimplement RPM
>> version checking if it is already provided by rpmdevtools:
>>
>> ~~~
>> $ /usr/bin/rpmdev-vercmp ipa-4.2.0-15.el7 4.2.0-15.el7_2.3; echo $?
>> WARNING: hyphen in release1: 4.2.0-15.el7
>>
>> rpmdev-vercmp  
>> rpmdev-vercmp  
>> rpmdev-vercmp # with no arguments, prompt
>>
>> Exit status is 0 if the EVR's are equal, 11 if EVR1 is newer, and
>> 12 if
>> EVR2
>> is newer.  Other exit statuses indicate problems.
>>
>> ipa-4.2.0-15.el7 < 4.2.0-15.el7_2.3
>> 12
>> ~~~
>>
>> which could be directly called from __eq__ or __lt__, since we are in
>> platform specific code anyway already.
>>
>> Martin
> Imho we should generally prefer reaching out to a Python library rather
> launching a subprocess to compare the versions, it's unnecessary
> overhead.
>>> I would not be too worried about miliseconds longer execution on a
>>> function
>>> called during RPM upgrade.
>>>
> That said, the main argument for the usage of rpm-python over
> rpmdevtools I see is that rpm-python is very likely to be present on
> the
> system (i.e. it is yum's own dependency), while rpmdevtools will not be
> present by default.
>
>   From the standpoint of trying to minimize the size of freeipa
> installation (i.e recent wget -> curl migration), we should keep the
> spirit here and do not introduce a dependency for a collection of
> developer tools.
>
> /2 cents
 +1, also a single function is hardly "much code".
>>> Ok then. If you all want to add yet another N-V-R parsing function in the
>>> FreeIPA code, I can live with it (with raised eyebrows though).
>>
>> Rebased patch attached.
> 
> I tested the patch, and it works fine - so conditional ACK from me for
> the current iteration of the patch, given developer consensus which was
> not reached yet.
> 
> There's a split of opinions (external binary camp vs. copy&paste camp),
> so we need to decide if we both camps are OK with proceeding.

Further inspection shows that rpmdevtools depends on Perl stack which seems to
be too much for such a simple thing. So, I'm hesitantly changing my NACK to
ACK for copy&paste camp.

-- 
Petr^2 Spacek

-- 
Manage your subscription for the Freeipa-dev

Re: [Freeipa-devel] [PATCH 0077] ipa-dns-install: Do not check for zone overlap when DNS, installed.

[David Kupka]

On 18/12/15 15:39, Petr Vobornik wrote:

On 12/18/2015 12:04 PM, Petr Vobornik wrote:

On 12/18/2015 11:26 AM, David Kupka wrote:

Standalone DNS installer always performed overlap check effectively
preventing installation on replica when other DNS instance was already
installed in topology.




I don't like the position of api argument in the install_check. It is
not consistent with install_check in KRA plus it's between two related
options: standalone and replica.

Is there a reason behind it which I don't see?


NACK, the API call is incorrect.

It should be api.Command['dns_is_enabled']()['result'] or
api.Command.dns_is_enabled()['result']


ipa-dns-install --forwarder=$DNS_FORWARDER

The log file for this installation can be found in
/var/log/ipaserver-install.log
Unexpected error - see /var/log/ipaserver-install.log for details:
TypeError: __call__() takes exactly 1 argument (2 given)


log file:

2015-12-18T14:33:30Z DEBUG   File
"/usr/lib/python2.7/site-packages/ipaserver/install/installutils.py",
line 736, in run_script
 return_value = main_function()

   File "/sbin/ipa-dns-install", line 137, in main
 dns_installer.install_check(True, api, False, options,
hostname=api.env.host)

   File "/usr/lib/python2.7/site-packages/ipaserver/install/dns.py",
line 113, in install_check
 already_enabled = api.Command('dns_is_enabled')['result']

2015-12-18T14:33:30Z DEBUG The ipa-dns-install command failed,
exception: TypeError: __call__() takes exactly 1 argument (2 given)

Thanks for catch. Sometimes I mess up with patch files. Fixed patch 
attached.


--
David Kupka
From 33a763f4aadb06d4c2c5741f69324d61969cad8a Mon Sep 17 00:00:00 2001
From: David Kupka 
Date: Thu, 17 Dec 2015 16:16:09 +0100
Subject: [PATCH] ipa-dns-install: Do not check for zone overlap when DNS
 installed.

When DNS is already installed somewhere in topology we should not check for
zone overlap because it would always say that we are overlapping our own domain.
ipa-replica-install already does that but ipa-dns-install did not.
---
 install/tools/ipa-dns-install  |  2 +-
 ipaserver/install/dns.py   | 24 
 ipaserver/install/server/install.py|  2 +-
 ipaserver/install/server/replicainstall.py |  4 ++--
 4 files changed, 12 insertions(+), 20 deletions(-)

diff --git a/install/tools/ipa-dns-install b/install/tools/ipa-dns-install
index eebc9e2ad8a6da71807b7d9d24cd41289c5b4d58..ee7ebe0bdcd2dde9ec1bcbe9f54dbe1baf3db9b2 100755
--- a/install/tools/ipa-dns-install
+++ b/install/tools/ipa-dns-install
@@ -134,7 +134,7 @@ def main():
 
 options.setup_ca = None  # must be None to enable autodetection
 
-dns_installer.install_check(True, False, options, hostname=api.env.host)
+dns_installer.install_check(True, api, False, options, hostname=api.env.host)
 dns_installer.install(True, False, options)
 
 # Restart http instance to make sure that python-dns has the right resolver
diff --git a/ipaserver/install/dns.py b/ipaserver/install/dns.py
index 763b2aca475d5f5b25d2aded05bc540ce3836f81..9a2fde29f613a3ce07b4f85f5ae2a856b806bdc8 100644
--- a/ipaserver/install/dns.py
+++ b/ipaserver/install/dns.py
@@ -99,20 +99,7 @@ def _disable_dnssec():
 conn.update_entry(entry)
 
 
-def check_dns_enabled(api):
-try:
-api.Backend.rpcclient.connect()
-result = api.Backend.rpcclient.forward(
-'dns_is_enabled',
-version=u'2.112',# All the way back to 3.0 servers
-)
-return result['result']
-finally:
-if api.Backend.rpcclient.isconnected():
-api.Backend.rpcclient.disconnect()
-
-
-def install_check(standalone, replica, options, hostname):
+def install_check(standalone, api, replica, options, hostname):
 global ip_addresses
 global reverse_zones
 fstore = sysrestore.FileStore(paths.SYSRESTORE)
@@ -121,8 +108,13 @@ def install_check(standalone, replica, options, hostname):
 raise RuntimeError("Integrated DNS requires '%s' package" %
constants.IPA_DNS_PACKAGE_NAME)
 
-# when installing first replica with DNS we need to check zone overlap
-if not replica or not check_dns_enabled(api):
+# when installing first DNS instance we need to check zone overlap
+if replica or standalone:
+already_enabled = api.Command.dns_is_enabled()['result']
+else:
+already_enabled = False
+
+if not already_enabled:
 domain = dnsutil.DNSName(util.normalize_zone(api.env.domain))
 print("Checking DNS domain %s, please wait ..." % domain)
 try:
diff --git a/ipaserver/install/server/install.py b/ipaserver/install/server/install.py
index a07ca664e263a1bb49c6f5e18bc888fa66e56b55..78cc5a4f5cfe1ee440392f9e0a7f5a508a32d4ab 100644
--- a/ipaserver/install/server/install.py
+++ b/ipaserver/install/server/install.py
@@ -706,7 +706,7 @@ def install_check(installer):
 sys.exit(1)
 
 if options.setup_dn

Re: [Freeipa-devel] [PATCH-0019][Tests]Enabled --auto-reverse by default for master installation

[Martin Basti]

On 22.12.2015 11:33, Oleg Fayans wrote:


On 12/21/2015 04:39 PM, Martin Basti wrote:


On 21.12.2015 15:02, Martin Basti wrote:


On 21.12.2015 11:25, Oleg Fayans wrote:

Hi Martin,

On 12/19/2015 08:02 PM, Martin Basti wrote:

On 18.12.2015 09:35, Oleg Fayans wrote:

Hi Petr,

On 12/17/2015 08:19 PM, Petr Spacek wrote:

On 17.12.2015 14:27, Oleg Fayans wrote:

Commit message was updated. Thanks!

On 12/17/2015 02:05 PM, Lukas Slebodnik wrote:

On (17/12/15 13:53), Oleg Fayans wrote:

--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.

>From ed4630140386c1043e36733eb42ec402cc276bee Mon Sep 17 00:00:00
2001

From: Oleg Fayans 
Date: Thu, 17 Dec 2015 13:50:19 +0100
Subject: [PATCH] Enabled automatic creation of reverse zone during
master
installation


The commit message does not contain ticket.
It is also not explained why this change was done.
Could you update commit message with verbose explanation?

Proper commit message might prevent removal of this change in
future
based on "git blame"

Seems reasonable in general but this should not happen in BRQ or
ABCDE labs.
Where do you see the problem?

Otherwise ACK.

The problem occurs in local libvirt-powered VMs. Also it may (or
may not
- I did not try it yet) be met in Beaker.



NACK

ipa-server-install: error: You cannot specify a --auto-reverse option
without the --setup-dns option

Agreed. Updated patch is attached.


ACK

Pushed to:
master: 36e85b10db7a8671c9116233ab4497ac6410a4a2
ipa-4-3: 9e3e51d354ff84098952b04ed98af0d93ae129e2


Question: shouldn't tasks.dns_install be called with --auto-reverse
option too?
Martin

[11:31:22]ofayans@ofayans:~/tmp/freeipa]$ grep "def dns_install"
ipatests/test_integration/tasks.py
[11:32:01]ofayans@ofayans:~/tmp/freeipa]$
[11:32:01]ofayans@ofayans:~/tmp/freeipa]$ echo $?
1
[11:32:32]ofayans@ofayans:~/tmp/freeipa]$

Maybe you meant something else?

Yes, I meant function dns_install in ipatests/test_integration/tasks.py 
module


--
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 0006] Refactor test_hostgroup_plugin

[Filip Skola]

And also sending refactored hostgroup plugin test.

FFrom a52f4f832a244ee53aa034d8d5ba645e83caa2e1 Mon Sep 17 00:00:00 2001
From: Filip Skola 
Date: Fri, 18 Dec 2015 15:25:21 +0100
Subject: [PATCH] Refactor test_hostgroup_plugin

---
 ipatests/test_xmlrpc/test_hostgroup_plugin.py | 377 +++---
 1 file changed, 100 insertions(+), 277 deletions(-)

diff --git a/ipatests/test_xmlrpc/test_hostgroup_plugin.py b/ipatests/test_xmlrpc/test_hostgroup_plugin.py
index 58665f466a18a4fb27c9f4e05cf4e89e1f48ccb1..61fda819b979f432150b95e54771130909b230d7 100644
--- a/ipatests/test_xmlrpc/test_hostgroup_plugin.py
+++ b/ipatests/test_xmlrpc/test_hostgroup_plugin.py
@@ -22,294 +22,117 @@
 Test the `ipalib.plugins.hostgroup` module.
 """
 
-from ipalib import api, errors
-from ipatests.test_xmlrpc.xmlrpc_test import Declarative, fuzzy_uuid
-from ipatests.test_xmlrpc import objectclasses
-from ipapython.dn import DN
+
+from ipatests.test_xmlrpc.xmlrpc_test import XMLRPC_test, raises_exact
+from ipatests.test_xmlrpc.tracker.hostgroup_plugin import HostGroupTracker
+from ipatests.test_xmlrpc.tracker.host_plugin import HostTracker
+from ipalib import errors
 import pytest
 
-hostgroup1 = u'testhostgroup1'
-dn1 = DN(('cn',hostgroup1),('cn','hostgroups'),('cn','accounts'),
- api.env.basedn)
 
-hostgroup_single = u'a'
-dn_single = DN(('cn',hostgroup_single),('cn','hostgroups'),('cn','accounts'),
- api.env.basedn)
+@pytest.fixture(scope='class')
+def hostgroup(request):
+tracker = HostGroupTracker(name=u'hostgroup')
+return tracker.make_fixture(request)
 
-fqdn1 = u'testhost1.%s' % api.env.domain
-host_dn1 = DN(('fqdn',fqdn1),('cn','computers'),('cn','accounts'),
-  api.env.basedn)
 
-invalidhostgroup1 = u'@invalid'
+@pytest.fixture(scope='class')
+def hostgroup_invalid(request):
+tracker = HostGroupTracker(name=u'@invalid')
+return tracker.make_fixture(request)
 
 
-@pytest.mark.tier1
-class test_hostgroup(Declarative):
+@pytest.fixture(scope='class')
+def hostgroup_single(request):
+tracker = HostGroupTracker(name=u'a')
+return tracker.make_fixture(request)
 
-cleanup_commands = [
-('hostgroup_del', [hostgroup1], {}),
-('host_del', [fqdn1], {}),
-]
 
-tests=[
+@pytest.fixture(scope='class')
+def host(request):
+tracker = HostTracker(name=u'host')
+return tracker.make_fixture(request)
 
-dict(
-desc='Try to retrieve non-existent %r' % hostgroup1,
-command=('hostgroup_show', [hostgroup1], {}),
-expected=errors.NotFound(
-reason=u'%s: host group not found' % hostgroup1),
-),
 
+class TestNonexistentHostGroup(XMLRPC_test):
+def test_retrieve_nonexistent(self, hostgroup):
+""" Try to retrieve non-existent hostgroup """
+hostgroup.ensure_missing()
+command = hostgroup.make_retrieve_command()
+with raises_exact(errors.NotFound(
+reason=u'%s: host group not found' % hostgroup.cn)):
+command()
 
-dict(
-desc='Try to update non-existent %r' % hostgroup1,
-command=('hostgroup_mod', [hostgroup1],
-dict(description=u'Updated hostgroup 1')
-),
-expected=errors.NotFound(
-reason=u'%s: host group not found' % hostgroup1),
-),
-
-
-dict(
-desc='Try to delete non-existent %r' % hostgroup1,
-command=('hostgroup_del', [hostgroup1], {}),
-expected=errors.NotFound(
-reason=u'%s: host group not found' % hostgroup1),
-),
-
-
-dict(
-desc='Test an invalid hostgroup name %r' % invalidhostgroup1,
-command=('hostgroup_add', [invalidhostgroup1], dict(description=u'Test')),
-expected=errors.ValidationError(name='hostgroup_name',
-error=u'may only include letters, numbers, _, -, and .'),
-),
-
-
-dict(
-desc='Create %r' % hostgroup1,
-command=('hostgroup_add', [hostgroup1],
-dict(description=u'Test hostgroup 1')
-),
-expected=dict(
-value=hostgroup1,
-summary=u'Added hostgroup "testhostgroup1"',
-result=dict(
-dn=dn1,
-cn=[hostgroup1],
-objectclass=objectclasses.hostgroup,
-description=[u'Test hostgroup 1'],
-ipauniqueid=[fuzzy_uuid],
-mepmanagedentry=[DN(('cn',hostgroup1),('cn','ng'),('cn','alt'),
-api.env.basedn)],
-),
-),
-),
-
-
-dict(
-desc='Try to create duplicate %r' % hostgroup1,
-command=('hostgroup_add', [hostgroup1],
-dict(description=u'Test hostgroup 1')
-),
-expected=errors.DuplicateEntry(message=
-

[Freeipa-devel] [PATCH 0005] Refactor test_nesting, create HostGroupTracker

[Filip Skola]

Hi,

another patch from refactoring-test_xmlrpc series.

Filip
From f2611f7d364657c0cf425d93b8a7262847e8b715 Mon Sep 17 00:00:00 2001
From: Filip Skola 
Date: Tue, 22 Dec 2015 11:33:32 +0100
Subject: [PATCH] Refactor test_nesting, create HostGroupTracker

---
 ipatests/test_xmlrpc/test_nesting.py   | 776 -
 ipatests/test_xmlrpc/tracker/group_plugin.py   |   4 +-
 ipatests/test_xmlrpc/tracker/host_plugin.py|   1 +
 .../{group_plugin.py => hostgroup_plugin.py}   | 222 +++---
 4 files changed, 255 insertions(+), 748 deletions(-)
 copy ipatests/test_xmlrpc/tracker/{group_plugin.py => hostgroup_plugin.py} (52%)

diff --git a/ipatests/test_xmlrpc/test_nesting.py b/ipatests/test_xmlrpc/test_nesting.py
index c3bf1ce84e0bef412c44ed847e7e0fc4648a4b74..f78a6e54bd7a94cb9d2645f5bdc5d5c109a79b1f 100644
--- a/ipatests/test_xmlrpc/test_nesting.py
+++ b/ipatests/test_xmlrpc/test_nesting.py
@@ -20,193 +20,93 @@
 Test group nesting and indirect members
 """
 
-from ipalib import api
-from ipatests.test_xmlrpc import objectclasses
-from ipatests.test_xmlrpc.xmlrpc_test import (Declarative, fuzzy_digits,
-  fuzzy_uuid)
-from ipapython.dn import DN
-from ipatests.test_xmlrpc.test_user_plugin import get_user_result
+from ipatests.test_xmlrpc.xmlrpc_test import XMLRPC_test
+from ipatests.test_xmlrpc.tracker.user_plugin import UserTracker
+from ipatests.test_xmlrpc.tracker.group_plugin import GroupTracker
+from ipatests.test_xmlrpc.tracker.host_plugin import HostTracker
+from ipatests.test_xmlrpc.tracker.hostgroup_plugin import HostGroupTracker
 import pytest
 
-group1 = u'testgroup1'
-group2 = u'testgroup2'
-group3 = u'testgroup3'
-group4 = u'testgroup4'
-user1 = u'tuser1'
-user2 = u'tuser2'
-user3 = u'tuser3'
-user4 = u'tuser4'
-
-hostgroup1 = u'testhostgroup1'
-hgdn1 = DN(('cn',hostgroup1),('cn','hostgroups'),('cn','accounts'),
-   api.env.basedn)
-hostgroup2 = u'testhostgroup2'
-hgdn2 = DN(('cn',hostgroup2),('cn','hostgroups'),('cn','accounts'),
-   api.env.basedn)
-
-fqdn1 = u'testhost1.%s' % api.env.domain
-host_dn1 = DN(('fqdn',fqdn1),('cn','computers'),('cn','accounts'),
-  api.env.basedn)
+
+@pytest.fixture(scope='class')
+def user1(request):
+tracker = UserTracker(name=u'tuser1', givenname=u'Test1', sn=u'User1')
+return tracker.make_fixture(request)
+
+
+@pytest.fixture(scope='class')
+def user2(request):
+tracker = UserTracker(name=u'tuser2', givenname=u'Test2', sn=u'User2')
+return tracker.make_fixture(request)
+
+
+@pytest.fixture(scope='class')
+def user3(request):
+tracker = UserTracker(name=u'tuser3', givenname=u'Test3', sn=u'User3')
+return tracker.make_fixture(request)
+
+
+@pytest.fixture(scope='class')
+def user4(request):
+tracker = UserTracker(name=u'tuser4', givenname=u'Test4', sn=u'User4')
+return tracker.make_fixture(request)
+
+
+@pytest.fixture(scope='class')
+def group1(request):
+tracker = GroupTracker(name=u'testgroup1', description=u'Test desc1')
+return tracker.make_fixture(request)
+
+
+@pytest.fixture(scope='class')
+def group2(request):
+tracker = GroupTracker(name=u'testgroup2', description=u'Test desc2')
+return tracker.make_fixture(request)
+
+
+@pytest.fixture(scope='class')
+def group3(request):
+tracker = GroupTracker(name=u'testgroup3', description=u'Test desc3')
+return tracker.make_fixture(request)
+
+
+@pytest.fixture(scope='class')
+def group4(request):
+tracker = GroupTracker(name=u'testgroup4', description=u'Test desc4')
+return tracker.make_fixture(request)
+
+
+@pytest.fixture(scope='class')
+def host1(request):
+tracker = HostTracker(name=u'host1')
+return tracker.make_fixture(request)
+
+
+@pytest.fixture(scope='class')
+def hostgroup1(request):
+tracker = HostGroupTracker(name=u'hostgroup1')
+return tracker.make_fixture(request)
+
+
+@pytest.fixture(scope='class')
+def hostgroup2(request):
+tracker = HostGroupTracker(name=u'hostgroup2')
+return tracker.make_fixture(request)
 
 
 @pytest.mark.tier1
-class test_nesting(Declarative):
-cleanup_commands = [
-('group_del', [group1], {}),
-('group_del', [group2], {}),
-('group_del', [group3], {}),
-('group_del', [group4], {}),
-('user_del', [user1], {}),
-('user_del', [user2], {}),
-('user_del', [user3], {}),
-('user_del', [user4], {}),
-('host_del', [fqdn1], {}),
-('hostgroup_del', [hostgroup1], {}),
-('hostgroup_del', [hostgroup2], {}),
-]
-
-tests = [
-
-
-# create group1:
-
-dict(
-desc='Create %r' % group1,
-command=(
-'group_add', [group1], dict(description=u'Test desc 1')
-),
-expected=dict(
-value=group1,
-summary=u'Added group "testgroup1"',
-result=dict(
-   

Re: [Freeipa-devel] [PATCH-0019][Tests]Enabled --auto-reverse by default for master installation

[Oleg Fayans]

On 12/21/2015 04:39 PM, Martin Basti wrote:
> 
> 
> On 21.12.2015 15:02, Martin Basti wrote:
>>
>>
>> On 21.12.2015 11:25, Oleg Fayans wrote:
>>> Hi Martin,
>>>
>>> On 12/19/2015 08:02 PM, Martin Basti wrote:

 On 18.12.2015 09:35, Oleg Fayans wrote:
> Hi Petr,
>
> On 12/17/2015 08:19 PM, Petr Spacek wrote:
>> On 17.12.2015 14:27, Oleg Fayans wrote:
>>> Commit message was updated. Thanks!
>>>
>>> On 12/17/2015 02:05 PM, Lukas Slebodnik wrote:
 On (17/12/15 13:53), Oleg Fayans wrote:
> -- 
> Oleg Fayans
> Quality Engineer
> FreeIPA team
> RedHat.
 >From ed4630140386c1043e36733eb42ec402cc276bee Mon Sep 17 00:00:00
 2001
> From: Oleg Fayans 
> Date: Thu, 17 Dec 2015 13:50:19 +0100
> Subject: [PATCH] Enabled automatic creation of reverse zone during
> master
> installation
>
 The commit message does not contain ticket.
 It is also not explained why this change was done.
 Could you update commit message with verbose explanation?

 Proper commit message might prevent removal of this change in
 future
 based on "git blame"
>> Seems reasonable in general but this should not happen in BRQ or
>> ABCDE labs.
>> Where do you see the problem?
>>
>> Otherwise ACK.
> The problem occurs in local libvirt-powered VMs. Also it may (or
> may not
> - I did not try it yet) be met in Beaker.
>
>
 NACK

 ipa-server-install: error: You cannot specify a --auto-reverse option
 without the --setup-dns option
>>> Agreed. Updated patch is attached.
>>>
>> ACK
>>
>> Pushed to:
>> master: 36e85b10db7a8671c9116233ab4497ac6410a4a2
>> ipa-4-3: 9e3e51d354ff84098952b04ed98af0d93ae129e2
>>
> Question: shouldn't tasks.dns_install be called with --auto-reverse
> option too?
> Martin

[11:31:22]ofayans@ofayans:~/tmp/freeipa]$ grep "def dns_install"
ipatests/test_integration/tasks.py
[11:32:01]ofayans@ofayans:~/tmp/freeipa]$
[11:32:01]ofayans@ofayans:~/tmp/freeipa]$ echo $?
1
[11:32:32]ofayans@ofayans:~/tmp/freeipa]$

Maybe you meant something else?

-- 
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.

-- 
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-0019][Tests]Enabled --auto-reverse by default for master installation

[Petr Spacek]

On 21.12.2015 16:39, Martin Basti wrote:
> 
> 
> On 21.12.2015 15:02, Martin Basti wrote:
>>
>>
>> On 21.12.2015 11:25, Oleg Fayans wrote:
>>> Hi Martin,
>>>
>>> On 12/19/2015 08:02 PM, Martin Basti wrote:

 On 18.12.2015 09:35, Oleg Fayans wrote:
> Hi Petr,
>
> On 12/17/2015 08:19 PM, Petr Spacek wrote:
>> On 17.12.2015 14:27, Oleg Fayans wrote:
>>> Commit message was updated. Thanks!
>>>
>>> On 12/17/2015 02:05 PM, Lukas Slebodnik wrote:
 On (17/12/15 13:53), Oleg Fayans wrote:
> -- 
> Oleg Fayans
> Quality Engineer
> FreeIPA team
> RedHat.
 >From ed4630140386c1043e36733eb42ec402cc276bee Mon Sep 17 00:00:00
 2001
> From: Oleg Fayans 
> Date: Thu, 17 Dec 2015 13:50:19 +0100
> Subject: [PATCH] Enabled automatic creation of reverse zone during
> master
> installation
>
 The commit message does not contain ticket.
 It is also not explained why this change was done.
 Could you update commit message with verbose explanation?

 Proper commit message might prevent removal of this change in future
 based on "git blame"
>> Seems reasonable in general but this should not happen in BRQ or
>> ABCDE labs.
>> Where do you see the problem?
>>
>> Otherwise ACK.
> The problem occurs in local libvirt-powered VMs. Also it may (or may not
> - I did not try it yet) be met in Beaker.
>
>
 NACK

 ipa-server-install: error: You cannot specify a --auto-reverse option
 without the --setup-dns option
>>> Agreed. Updated patch is attached.
>>>
>> ACK
>>
>> Pushed to:
>> master: 36e85b10db7a8671c9116233ab4497ac6410a4a2
>> ipa-4-3: 9e3e51d354ff84098952b04ed98af0d93ae129e2
>>
> Question: shouldn't tasks.dns_install be called with --auto-reverse option 
> too?
> Martin

Yeah, it probably should.

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