On 16.10.2015 12:36, Jan Cholasta wrote:
On 16.10.2015 10:31, Martin Basti wrote:
On 16.10.2015 10:05, Martin Babinsky wrote:
On 10/16/2015 08:47 AM, Jan Cholasta wrote:
On 16.10.2015 08:42, Martin Kosek wrote:
On 10/16/2015 06:00 AM, Jan Cholasta wrote:
On 15.10.2015 19:47, Martin Basti wrote:
On 15.10.2015 18:47, Martin Basti wrote:
On 15.10.2015 18:36, Martin Babinsky wrote:
On 10/15/2015 04:50 PM, Martin Basti wrote:
On 14.10.2015 16:10, Martin Basti wrote:
On 14.10.2015 15:51, Martin Babinsky wrote:
On 10/13/2015 06:38 PM, Martin Basti wrote:
On 12.10.2015 12:30, Martin Babinsky wrote:
On 10/08/2015 05:58 PM, Martin Basti wrote:
The attached patches fix following tickets:
https://fedorahosted.org/freeipa/ticket/4949
https://fedorahosted.org/freeipa/ticket/4048
https://fedorahosted.org/freeipa/ticket/1930
With these patches, an administrator can specify LDIF file
that
contains
modifications to be applied to dse.ldif right after
creation
of DS
instance.
Hi,
Functionally the paches work as expected. However I have
following
nitpicks:
in patch 318:
1.) there is a typo in ModifyLDIF class docstring:
+ Operations keep the order in whihc were specified
per DN.
in patch 320:
1.) you should use 'os.path.join' to construct FS paths:
- dse_filename = '%s/%s' % (
+ dse_filename = os.path.join(
paths.ETC_DIRSRV_SLAPD_INSTANCE_TEMPLATE %
self.serverid,
- 'dse.ldif',
+ 'dse.ldif'
)
2.) IIUC the 'config_ldif_file' knob in BaseServer holds the
path to
LDIF containing the mod operations to dse.ldif. However, the
knob name
sounds like the option accepts the path of dse.ldif
itself. I
propose
to rename the knob to something more in-line with the
supposed
function, like 'dse_mods_file'.
Updated patches + CI test attached
Patches work as expected and CI tests are OK.
However it seems that warning level messages are not logged to
installer output but only to log file (*waves hand* magic).
Maybe it would be a good idea to raise the message level to
"error",
so that it is immediately obvious to the user that his DSE
mods
contain an error and cannot be parsed.
Also you have a typo in the commit message of patch 320
(s/chages/changes/).
Updated patches attached.
Rebased patches for master branch.
ACK
Pushed to master: 5233165ce7062bb7aa649bf95a029103c375207b
Pushed to ipa-4-2: 94412b81c1c09b56ee2900942a1b804f21c264c5
These tickets are not for ipa-4-2,
reverted a17936a1aad139236f18f0e5ad0b066f7747ca60
Can we use a better option name? --dirsrv-config-mods sounds weird,
as you need
to specify a file, not mods...
+1. maybe --dirsrv-config-ldif?
--dirsrv-config-file is most consistent with other options which
accept
files (--external-cert-file, --ca-cert-file, --kasp-db-file, etc.)
This, however, may be confusing to user since '--dirsrv-config-file'
may evoke a feeling that it consumes *whole new* dse.ldif while in
reality it is only a few custom mods to directory server configuration.
I agree, it expects only file containing modifications in LDIF format,
'config-file' suffix may be confusing
Sorry, but this does not make any sense. Why would anyone think they
are supposed to specify a complete dse.ldif? Is it written somewhere
that DS config file == dse.ldif? I don't think so. And, if you use
--help, you will see exactly what the option does right away.
What is actually confusing is inventing a "smart" name instead of
making it consistent with everything else.
Patch attached.
Martin^2
Speaking about the option, I saw some unescaped
"-"'s in the man page updates:
+.TP
+\fB\-\-dirsrv-config-mods\fR
+The path to LDIF file that will be used to modify configuration of
dse.ldif
during installation of the directory server instance
Martin
From 3a78ba53300ea0eba2760edc34076a74b2de970a Mon Sep 17 00:00:00 2001
From: Martin Basti <mba...@redhat.com>
Date: Fri, 16 Oct 2015 13:43:43 +0200
Subject: [PATCH] Rename option --dirsrv-config-mods to --dirsrv-config-file
Option is renamed to be consistent with other options.
Affected tickets:
https://fedorahosted.org/freeipa/ticket/4949
https://fedorahosted.org/freeipa/ticket/4048
https://fedorahosted.org/freeipa/ticket/1930
---
install/tools/man/ipa-replica-install.1 | 2 +-
install/tools/man/ipa-server-install.1 | 2 +-
ipaserver/install/server/common.py | 6 +++---
ipaserver/install/server/install.py | 4 ++--
ipaserver/install/server/replicainstall.py | 2 +-
ipatests/test_integration/test_customized_ds_config_install.py | 4 ++--
6 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/install/tools/man/ipa-replica-install.1 b/install/tools/man/ipa-replica-install.1
index 12a5dd7b85af30023614567e9d70073082bda9c1..df01c616c89abeb5c1f421c6e29b1034b2ec9ea7 100644
--- a/install/tools/man/ipa-replica-install.1
+++ b/install/tools/man/ipa-replica-install.1
@@ -75,7 +75,7 @@ Enable debug logging when more verbose output is needed
\fB\-U\fR, \fB\-\-unattended\fR
An unattended installation that will never prompt for user input
.TP
-\fB\-\-dirsrv-config-mods\fR
+\fB\-\-dirsrv\-config\-file\fR
The path to LDIF file that will be used to modify configuration of dse.ldif during installation of the directory server instance
.SS "CERTIFICATE SYSTEM OPTIONS"
diff --git a/install/tools/man/ipa-server-install.1 b/install/tools/man/ipa-server-install.1
index ba43c80a6589aedea01111c9889d6710adf7d35e..5d0b96034b34ed73274ac58c62e7b0f2e9941014 100644
--- a/install/tools/man/ipa-server-install.1
+++ b/install/tools/man/ipa-server-install.1
@@ -79,7 +79,7 @@ Enable debug logging when more verbose output is needed
\fB\-U\fR, \fB\-\-unattended\fR
An unattended installation that will never prompt for user input
.TP
-\fB\-\-dirsrv-config-mods\fR
+\fB\-\-dirsrv\-config\-file\fR
The path to LDIF file that will be used to modify configuration of dse.ldif during installation of the directory server instance
.SS "CERTIFICATE SYSTEM OPTIONS"
diff --git a/ipaserver/install/server/common.py b/ipaserver/install/server/common.py
index a09f394d613429818d9a09c14d1cfa6b7fe67186..93c95dd8e8d2b24af193ee19368959188bcd6cb9 100644
--- a/ipaserver/install/server/common.py
+++ b/ipaserver/install/server/common.py
@@ -343,7 +343,7 @@ class BaseServer(common.Installable, common.Interactive, core.Composite):
description="Do not automatically create DNS SSHFP records",
)
- dirsrv_config_mods = Knob(
+ dirsrv_config_file = Knob(
str, None,
description="The path to LDIF file that will be used to modify "
"configuration of dse.ldif during installation of the "
@@ -351,8 +351,8 @@ class BaseServer(common.Installable, common.Interactive, core.Composite):
cli_metavar='FILE',
)
- @dirsrv_config_mods.validator
- def dirsrv_config_mods(self, value):
+ @dirsrv_config_file.validator
+ def dirsrv_config_file(self, value):
if not os.path.exists(value):
raise ValueError("File %s does not exist." % value)
diff --git a/ipaserver/install/server/install.py b/ipaserver/install/server/install.py
index 566af69a29d62023b5bc2d015c8cc3d490983f51..3f8ba2027ac210cc5cc9cc706c5d39e01e6de7e4 100644
--- a/ipaserver/install/server/install.py
+++ b/ipaserver/install/server/install.py
@@ -735,7 +735,7 @@ def install(installer):
if options.dirsrv_cert_files:
ds = dsinstance.DsInstance(fstore=fstore,
domainlevel=options.domainlevel,
- config_ldif=options.dirsrv_config_mods)
+ config_ldif=options.dirsrv_config_file)
installer._ds = ds
ds.create_instance(realm_name, host_name, domain_name,
dm_password, dirsrv_pkcs12_info,
@@ -745,7 +745,7 @@ def install(installer):
else:
ds = dsinstance.DsInstance(fstore=fstore,
domainlevel=options.domainlevel,
- config_ldif=options.dirsrv_config_mods)
+ config_ldif=options.dirsrv_config_file)
installer._ds = ds
ds.create_instance(realm_name, host_name, domain_name,
dm_password,
diff --git a/ipaserver/install/server/replicainstall.py b/ipaserver/install/server/replicainstall.py
index 071b12bc883bef7d7dce810dfefa2034427d91a7..74349f605c7e21c334f3271bcbb703b7cafd7e9b 100644
--- a/ipaserver/install/server/replicainstall.py
+++ b/ipaserver/install/server/replicainstall.py
@@ -96,7 +96,7 @@ def install_replica_ds(config, options, promote=False):
pkcs12_info = make_pkcs12_info(config.dir, "dscert.p12", "dirsrv_pin.txt")
ds = dsinstance.DsInstance(
- config_ldif=options.dirsrv_config_mods)
+ config_ldif=options.dirsrv_config_file)
ds.create_replica(
realm_name=config.realm_name,
master_fqdn=config.master_host_name,
diff --git a/ipatests/test_integration/test_customized_ds_config_install.py b/ipatests/test_integration/test_customized_ds_config_install.py
index d55f6b9c8a6bc415b770de101334d2738242d7cc..0d8c836d9c511965d4158044a4bcecfdd7774800 100644
--- a/ipatests/test_integration/test_customized_ds_config_install.py
+++ b/ipatests/test_integration/test_customized_ds_config_install.py
@@ -64,7 +64,7 @@ class TestCustomInstallMaster(IntegrationTest):
'-r', self.master.domain.name,
'-p', self.master.config.dirman_password,
'-a', self.master.config.admin_password,
- '--dirsrv-config-mods', CONFIG_LDIF_PATH,
+ '--dirsrv-config-file', CONFIG_LDIF_PATH,
]
self.master.run_command(args)
@@ -89,6 +89,6 @@ class TestCustomInstallReplica(IntegrationTest):
'-p', self.replicas[0].config.dirman_password,
'-w', self.replicas[0].config.admin_password,
'--ip-address', self.replicas[0].ip,
- '--dirsrv-config-mods', CONFIG_LDIF_PATH,
+ '--dirsrv-config-file', CONFIG_LDIF_PATH,
replica_filename]
self.replicas[0].run_command(args)
--
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
