Re: [Freeipa-devel] [PATCHES 466-468, 0316] install: Add common base class for server and replica install

2015-10-01 Thread Jan Cholasta 

On 29.9.2015 15:15, Martin Basti wrote:



On 09/29/2015 03:11 PM, Milan Kubík wrote:

On 09/23/2015 05:01 PM, Martin Basti wrote:



On 09/22/2015 12:10 PM, Jan Cholasta wrote:

On 22.9.2015 10:29, Martin Babinsky wrote:

On 09/16/2015 10:44 AM, Jan Cholasta wrote:

On 16.9.2015 08:11, Jan Cholasta wrote:

On 15.9.2015 07:22, Jan Cholasta wrote:

On 10.8.2015 16:58, Martin Babinsky wrote:

On 08/06/2015 08:22 AM, Jan Cholasta wrote:

Hi,

the attached patch fixes part of
.

See also Martin Babinsky's patch 51:
.







Honza



Sorry but NACK, see below:

1.) it seems that passing kwargs to Server components doesn't
work as
expected. See these logs (install on fresh F22 VM):

http://fpaste.org/253416/21363814/
http://fpaste.org/253419/43921374/


Fixed.



2.) the following code blows up in BaseServers' __init__:
(http://fpaste.org/253400/21225314/)

392 if not self.dns.setup_dns:
393 if self.dns.forwarders:
394 raise RuntimeError(
395 "You cannot specify a --forwarder option
without
the "
396 "--setup-dns option")


I think that the check should be:

392 if not self.setup_dns:
393 if self.dns.forwarders:


Fixed.



IMHO BaseServerDNS class shouldn't have setup_dns knob, that
should be
set in the parent class (BaseServer)


Fixed.



3.) Is there any reason why BaseServer doesn't have
'master_password',
'idmax' and 'idstart' knobs? I know that these are then brought
in by
the derived Server class, but the check for them is in parent's
__init__() method and it is IMHO a bit confusing


The check should be in Server, fixed.



4.) please add license header to the beginning of
'ipaserver/install/server/common.py' file


Added.

Updated patches attached.


Self-NACK, I broke ipa-server-install --uninstall.


Fixed.



ACK to all three patches.



Thanks.

Pushed to:
master: 86edd6abeb9749e159a529b83cfce6443fff4ba5
ipa-4-2: 42d16b02cd153ac89ebd8ae07c98611dc3b6e471


These patches introduced regression.
ipa-replica-install in unattended mode requires to specify -a, -p and
-r options.

Attached patch fixes it.




Works for me, ACK.

Milan

Pushed to ipa-4-2: ad285897f54190fd0113209f32fce7f37fb0ce77
Pushed to master: 74da4f5870edda85039b3bba52fb0a578676fb44


Martin found an additional issued, see the attached patch for a fix.

--
Jan Cholasta
From ef371ec68e8b41feae70d9bd9c6155a01a6e2f85 Mon Sep 17 00:00:00 2001
From: Jan Cholasta 
Date: Thu, 1 Oct 2015 11:41:39 +0200
Subject: [PATCH] install: fix ipa-server-install fail on missing --forwarder

https://fedorahosted.org/freeipa/ticket/4517
---
 ipaserver/install/server/common.py | 4 
 ipaserver/install/server/install.py| 6 ++
 ipaserver/install/server/replicainstall.py | 7 +++
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/ipaserver/install/server/common.py b/ipaserver/install/server/common.py
index 0648b40..3eb7279 100644
--- a/ipaserver/install/server/common.py
+++ b/ipaserver/install/server/common.py
@@ -401,10 +401,6 @@ class BaseServer(common.Installable, common.Interactive, core.Composite):
 raise RuntimeError(
 "You cannot specify a --forwarder option together with "
 "--no-forwarders")
-elif not self.dns.forwarders and not self.dns.no_forwarders:
-raise RuntimeError(
-"You must specify at least one --forwarder option or "
-"--no-forwarders option")
 elif self.dns.reverse_zones and self.dns.no_reverse:
 raise RuntimeError(
 "You cannot specify a --reverse-zone option together with "
diff --git a/ipaserver/install/server/install.py b/ipaserver/install/server/install.py
index 4fe1ed9..32eef1c 100644
--- a/ipaserver/install/server/install.py
+++ b/ipaserver/install/server/install.py
@@ -1269,6 +1269,12 @@ class Server(BaseServer):
 raise RuntimeError(
 "In unattended mode you need to provide at least -r, -p "
 "and -a options")
+if self.setup_dns:
+#pylint: disable=no-member
+if not self.dns.forwarders and not self.dns.no_forwarders:
+raise RuntimeError(
+"You must specify at least one --forwarder option or "
+"--no-forwarders option")
 
 if self.idmax < self.idstart:
 raise RuntimeError(
diff --git a/ipaserver/install/server/replicainstall.py b/ipaserver/install/server/replicainstall.py
index 79bbcda..3087091 100644
--- a/ipaserver/install/server/replicainstall.py
+++ b/ipaserver/install/server/replicainstall.py
@@ -741,6 +741,13 @@ class Replica(BaseServer):
 raise RuntimeError(
 "Replica file %s does not exist" % self.replica_file)

Re: [Freeipa-devel] [PATCHES 466-468, 0316] install: Add common base class for server and replica install

2015-09-29 Thread Milan Kubík 

On 09/23/2015 05:01 PM, Martin Basti wrote:



On 09/22/2015 12:10 PM, Jan Cholasta wrote:

On 22.9.2015 10:29, Martin Babinsky wrote:

On 09/16/2015 10:44 AM, Jan Cholasta wrote:

On 16.9.2015 08:11, Jan Cholasta wrote:

On 15.9.2015 07:22, Jan Cholasta wrote:

On 10.8.2015 16:58, Martin Babinsky wrote:

On 08/06/2015 08:22 AM, Jan Cholasta wrote:

Hi,

the attached patch fixes part of
.

See also Martin Babinsky's patch 51:
. 








Honza



Sorry but NACK, see below:

1.) it seems that passing kwargs to Server components doesn't 
work as

expected. See these logs (install on fresh F22 VM):

http://fpaste.org/253416/21363814/
http://fpaste.org/253419/43921374/


Fixed.



2.) the following code blows up in BaseServers' __init__:
(http://fpaste.org/253400/21225314/)

392 if not self.dns.setup_dns:
393 if self.dns.forwarders:
394 raise RuntimeError(
395 "You cannot specify a --forwarder option
without
the "
396 "--setup-dns option")


I think that the check should be:

392 if not self.setup_dns:
393 if self.dns.forwarders:


Fixed.



IMHO BaseServerDNS class shouldn't have setup_dns knob, that 
should be

set in the parent class (BaseServer)


Fixed.



3.) Is there any reason why BaseServer doesn't have 
'master_password',
'idmax' and 'idstart' knobs? I know that these are then brought 
in by

the derived Server class, but the check for them is in parent's
__init__() method and it is IMHO a bit confusing


The check should be in Server, fixed.



4.) please add license header to the beginning of
'ipaserver/install/server/common.py' file


Added.

Updated patches attached.


Self-NACK, I broke ipa-server-install --uninstall.


Fixed.



ACK to all three patches.



Thanks.

Pushed to:
master: 86edd6abeb9749e159a529b83cfce6443fff4ba5
ipa-4-2: 42d16b02cd153ac89ebd8ae07c98611dc3b6e471


These patches introduced regression.
ipa-replica-install in unattended mode requires to specify -a, -p and 
-r options.


Attached patch fixes it.




Works for me, ACK.

Milan
-- 
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] [PATCHES 466-468, 0316] install: Add common base class for server and replica install

2015-09-23 Thread Martin Basti 



On 09/22/2015 12:10 PM, Jan Cholasta wrote:

On 22.9.2015 10:29, Martin Babinsky wrote:

On 09/16/2015 10:44 AM, Jan Cholasta wrote:

On 16.9.2015 08:11, Jan Cholasta wrote:

On 15.9.2015 07:22, Jan Cholasta wrote:

On 10.8.2015 16:58, Martin Babinsky wrote:

On 08/06/2015 08:22 AM, Jan Cholasta wrote:

Hi,

the attached patch fixes part of
.

See also Martin Babinsky's patch 51:
. 








Honza



Sorry but NACK, see below:

1.) it seems that passing kwargs to Server components doesn't 
work as

expected. See these logs (install on fresh F22 VM):

http://fpaste.org/253416/21363814/
http://fpaste.org/253419/43921374/


Fixed.



2.) the following code blows up in BaseServers' __init__:
(http://fpaste.org/253400/21225314/)

392 if not self.dns.setup_dns:
393 if self.dns.forwarders:
394 raise RuntimeError(
395 "You cannot specify a --forwarder option
without
the "
396 "--setup-dns option")


I think that the check should be:

392 if not self.setup_dns:
393 if self.dns.forwarders:


Fixed.



IMHO BaseServerDNS class shouldn't have setup_dns knob, that 
should be

set in the parent class (BaseServer)


Fixed.



3.) Is there any reason why BaseServer doesn't have 
'master_password',
'idmax' and 'idstart' knobs? I know that these are then brought 
in by

the derived Server class, but the check for them is in parent's
__init__() method and it is IMHO a bit confusing


The check should be in Server, fixed.



4.) please add license header to the beginning of
'ipaserver/install/server/common.py' file


Added.

Updated patches attached.


Self-NACK, I broke ipa-server-install --uninstall.


Fixed.



ACK to all three patches.



Thanks.

Pushed to:
master: 86edd6abeb9749e159a529b83cfce6443fff4ba5
ipa-4-2: 42d16b02cd153ac89ebd8ae07c98611dc3b6e471


These patches introduced regression.
ipa-replica-install in unattended mode requires to specify -a, -p and -r 
options.


Attached patch fixes it.

From ff5c1247520ad2d97047d28cc3f94e76faf55301 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Wed, 23 Sep 2015 15:48:30 +0200
Subject: [PATCH] Replica inst. fix: do not require -r, -a, -p options in
 unattended mode

Previous patches for this ticket introduced error, that replica install
requires to specify -r, -p and -a option in unattended mode.
This options are not needed on replica side.

https://fedorahosted.org/freeipa/ticket/4517
---
 ipaserver/install/server/common.py  | 7 ---
 ipaserver/install/server/install.py | 6 ++
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/ipaserver/install/server/common.py b/ipaserver/install/server/common.py
index e7fb2acfc0eb36547f175fef51fcb5c7f5764269..0648b40e550309195f7bc88e2f5f29af4139d5d5 100644
--- a/ipaserver/install/server/common.py
+++ b/ipaserver/install/server/common.py
@@ -348,13 +348,6 @@ class BaseServer(common.Installable, common.Interactive, core.Composite):
 
 #pylint: disable=no-member
 
-if not self.uninstalling and not self.interactive:
-if (not self.realm_name or not self.dm_password or
-not self.admin_password):
-raise RuntimeError(
-"In unattended mode you need to provide at least -r, -p "
-"and -a options")
-
 # If any of the key file options are selected, all are required.
 cert_file_req = (self.ca.dirsrv_cert_files, self.ca.http_cert_files)
 cert_file_opt = (self.ca.pkinit_cert_files,)
diff --git a/ipaserver/install/server/install.py b/ipaserver/install/server/install.py
index 9137e1a9b0cd77803fefaf60db82606eafa42d76..4fe1ed9f25206e7c014e544fcc3e71243e685f86 100644
--- a/ipaserver/install/server/install.py
+++ b/ipaserver/install/server/install.py
@@ -1263,6 +1263,12 @@ class Server(BaseServer):
 self.master_password):
 raise RuntimeError(
 "In uninstall mode, -a, -r and -P options are not allowed")
+elif not self.interactive:
+if (not self.realm_name or not self.dm_password or
+not self.admin_password):
+raise RuntimeError(
+"In unattended mode you need to provide at least -r, -p "
+"and -a options")
 
 if self.idmax < self.idstart:
 raise RuntimeError(
-- 
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