On 08/03/2015 01:56 PM, Martin Babinsky wrote:
On 07/30/2015 08:55 AM, Jan Cholasta wrote:
Dne 29.7.2015 v 17:43 Petr Vobornik napsal(a):
On 07/29/2015 05:13 PM, Martin Babinsky wrote:
On 07/29/2015 01:25 PM, Jan Cholasta wrote:
Dne 29.7.2015 v 12:20 Martin Babinsky napsal(a):
Initial attempt to implement
https://fedorahosted.org/freeipa/ticket/4517
Some points to discuss:
1.) name of the config entries: currently the option names are
derived
from CLI options but have underscores in them instead of dashes.
Maybe
keeping the CLI option names also for config entries will make it
easier
for the user to transfer their CLI options from scripts to config
files.
NACK. There is no point in generating config names from CLI names,
which
are generated from knob names - use knob names directly.
The problem is that in some cases the cli_name does not map
directly to
knob name, leading in different naming of CLI options and config
entries, confusion and mayhem.
What works for CLI may not work for config files and vice versa. For
example, this works for CLI:
--no-ntp
--no-forwarders
--forwarder 1.2.3.4 --forwarder 5.6.7.8
but this works better in config file:
ntp = False
forwarders =
forwarders = 1.2.3.4, 5.6.7.8
These are some offenders from `ipaserver/install/server.py`:
http://fpaste.org/249424/18226114/
On the other hand, this can be an incentive to finally put an end to
inconsistent option/knob naming across server/replica/etc. installers.
Yes please.
If the names are different than cli names, then they should be made
discoverable somehow or be documented.
IMHO documenting them is easy.
2.) Config sections: there is currently only one valid section named
'[global]' in accordance with the format of 'default.conf'. Should we
have separate sections equivalent to option groups in CLI (e.g.
[basic],
[certificate system], [dns])?
No, because they would have to be maintained forever. For example,
some
options are in wrong sections and we wouldn't be able to move them.
I'm also more inclined to a single section, at least for now since we
are pressed for time with this RFE.
That's not to say that we should ditch Alexander's idea about separate
sections with overrides for different hosts. We should consider it as a
future enhancement to this feature once the basic plumbing is in place.
Right.
3.) Handling of unattended mode when specifying a config file:
Currently there is no connection between --config-file and unattended
mode. So when you run ipa-server-install using config file, you still
get asked for missing stuff. Should '--config-file' automatically
imply
'--unattended'?
The behavior should be the same as if you specified the options on the
command line. So no, --config-file should not imply --unattended.
That sound reasonable. the code behaves this way already so no changes
here.
There are probably other issues to discuss. Feel free to write
email/ping me on IRC.
(I haven't looked at the patch yet.)
Please take a look at it ASAP. I am on PTO tomorrow and on Friday,
but I
will find time to work at it in the evening if you send me you
comments.
1) IMO the option should be in the top-level option section, not in a
separate group (use "parser.add_option()").
Also maybe rename it to --config, AFAIK that's what is usually used.
A short name ("-c"?) would be nice too.
Nitpick: if the option is named --config-file, dest should be
"config_file", to make it easier to look it up in the code.
2) Please don't duplicate the knob retrieval code, store knobs in a list
and pass that as an argument to parse_config_file.
3) I'm not sure about using newline as a list separator. I don't know
about other IPA components, but SSSD in particular uses commas, maybe we
should be consistent with that?
4) Booleans should be assignable either True or False, i.e. do not use
_parse_knob to parse them.
Honza
Attaching updated patch.
Attaching updated patch.
I have also sneaked in a small change that improves the testability of
installers (making configurator a member of ConfigureTool instance).
Should I send a separate patch for that?
--
Martin^3 Babinsky
From c73cae8dd0a189a2384f010b81273d88fc450f8d Mon Sep 17 00:00:00 2001
From: Martin Babinsky <mbabi...@redhat.com>
Date: Wed, 22 Jul 2015 13:55:26 +0200
Subject: [PATCH 1/2] IPA server and replica installers can accept options from
config file
New option '-c'/'--config' enables ipa-server-install and ipa-replica-install
to obtain parameters from supplied configuration file in INI format.
The file syntax is as follows:
* all options are listed in a single [global] section
* the option name can be derived from long CLI option name by replacing
dashes with underscores
* to specify a multivalued parameter, assign a list of comma separated
values to a single option. Escape commas in values like DNs by a
backslash.
Parameters specified explicitly through CLI options take precedence over the
values contained in config file.
In the case of unknown options present in the config file, the installer will
raise an error listing them.
https://fedorahosted.org/freeipa/ticket/4517
---
ipapython/install/cli.py | 91 ++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 88 insertions(+), 3 deletions(-)
diff --git a/ipapython/install/cli.py b/ipapython/install/cli.py
index 1ba9a815c4c499dff0e7974f399f2de31eb932cd..98297aa3d3361dbaa08529340e69e034fb04a4f8 100644
--- a/ipapython/install/cli.py
+++ b/ipapython/install/cli.py
@@ -9,6 +9,8 @@ Command line support.
import collections
import optparse
import signal
+from ConfigParser import RawConfigParser
+from csv import reader, QUOTE_NONE
from ipapython import admintool, ipa_log_manager
from ipapython.ipautil import CheckedIPAddress, private_ccache
@@ -71,6 +73,7 @@ class ConfigureTool(admintool.AdminTool):
configurable_class = None
debug_option = False
positional_arguments = None
+ cfgr = None
@staticmethod
def _transform(configurable_class):
@@ -78,6 +81,15 @@ class ConfigureTool(admintool.AdminTool):
@classmethod
def add_options(cls, parser):
+ parser.add_option(
+ "-c",
+ "--config",
+ dest='config',
+ default=None,
+ metavar='FILE',
+ help="get installer specific options from config file"
+ )
+
basic_group = optparse.OptionGroup(parser, "basic options")
groups = collections.OrderedDict()
@@ -275,7 +287,16 @@ class ConfigureTool(admintool.AdminTool):
kwargs = {}
transformed_cls = self._transform(self.configurable_class)
- for owner_cls, name in transformed_cls.knobs():
+ knob_list = [(getattr(owner_cls, name), name) for owner_cls, name
+ in transformed_cls.knobs()]
+
+ if self.options.config is not None:
+ values_from_config = self.parse_config_file(
+ knob_list,
+ self.options.config)
+ kwargs.update(values_from_config)
+
+ for knob_cls, name in knob_list:
value = getattr(self.options, name, None)
if value is not None:
kwargs[name] = value
@@ -285,7 +306,7 @@ class ConfigureTool(admintool.AdminTool):
kwargs['interactive'] = True
try:
- cfgr = transformed_cls(**kwargs)
+ self.cfgr = transformed_cls(**kwargs)
except core.KnobValueError as e:
knob_cls = getattr(transformed_cls, e.name)
try:
@@ -305,12 +326,76 @@ class ConfigureTool(admintool.AdminTool):
with private_ccache():
super(ConfigureTool, self).run()
- cfgr.run()
+ self.cfgr.run()
@staticmethod
def __signal_handler(signum, frame):
raise KeyboardInterrupt
+ def parse_config_file(self, knob_list, filename):
+ config_parser = RawConfigParser()
+
+ with open(filename, 'r') as f:
+ config_parser.readfp(f)
+
+ # use single section name for now
+ section_name = 'global'
+ config_options = {x for x in config_parser.options(section_name)}
+
+ result = {}
+
+ for knob_cls, name in knob_list:
+
+ try:
+ config_options.remove(name)
+ except KeyError:
+ continue
+
+ try:
+ result[name] = self._value_from_config(knob_cls,
+ config_parser,
+ section_name,
+ name)
+ except Exception as e:
+ raise RuntimeError(
+ "{0}: {1}".format(
+ filename, e)
+ )
+
+ # check for unrecognized entries left and raise an error
+ if config_options:
+ raise RuntimeError(
+ "{0}: Unrecognized options: {1}".format(
+ filename, ', '.join(sorted(config_options))
+ )
+ )
+
+ return result
+
+ def _value_from_config(self, knob_cls, parser, section, option):
+ # since ConfigParser does not support multi-valued entries, we will use
+ # csv reader to parse comma-separated values when multi-valued Knob is
+ # detected. Use backslash as escape character to safely handle values
+ # with commas such as DNs
+ if isinstance(knob_cls.type, tuple) and knob_cls.type[0] == list:
+ raw_value = parser.get(section, option)
+
+ csv_reader = reader([raw_value], skipinitialspace=True,
+ strict=True, quoting=QUOTE_NONE,
+ escapechar='\\')
+ csv_fields = csv_reader.next()
+ new_value = None
+ for v in csv_fields:
+ new_value = self._parse_knob(
+ knob_cls, new_value, v)
+ elif knob_cls.type is bool:
+ new_value = parser.getboolean(section, option)
+ else:
+ new_value = self._parse_knob(knob_cls, None,
+ parser.get(section, option))
+
+ return new_value
+
class InstallTool(ConfigureTool):
uninstall_kwargs = None
--
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
