Re: [Freeipa-devel] [PATCH 0095-0098] NTP: use augeas, configure chronyd, do not overwrite config

[Jan Cholasta]

On 20.5.2016 14:54, Martin Babinsky wrote:

On 05/20/2016 02:29 PM, Martin Babinsky wrote:

On 05/16/2016 01:58 PM, David Kupka wrote:

On 26/04/16 10:09, David Kupka wrote:

On 14/03/16 14:01, Martin Basti wrote:



On 14.03.2016 13:46, Martin Babinsky wrote:

On 03/11/2016 09:16 AM, David Kupka wrote:

Current version (0.5.0) of python-augeas is missing copy() method.
Use
dkupka/python-augeas copr repo before new version it's build and
available in the official repos.




Hi David,

TLDR: NACK :D.

Here are high-level remarks to discuss:

Maybe it would be a good idea to move ipaaugeas/changeconf and ntp to
ipaplatform since it is dealing with (sorta) platform specific
behavior (ntp vs. chrony vs. whatever we will have for timesync in
the
future). CC'ing Jan for thoughts.

Also regarding patches 0096-0097, we could have platform specific
TimeDateService object that could wrap NTP/chrony management. for
example, the task namespace functions in Pathc 0096 could be
reimplemented as a methods of the service (RedhatTimeDateService,
FedoraTimeDateService etc.) and then called in a platform-agnostic
manner.

Here are some comments regarding code:

Patch 0095:

1.)
+IPA_CUSTOM_AUGEAS_LENSES_DIR = '/usr/share/augeas/lenses/ipa/'

Do not forget to add this directory to %install and %files
spection of
the spec file so that it is correctly added to RPM build.

2.)

please separate import of system-wide and IPA-specific modules by
blank line

+import collections
+from augeas import Augeas
+from ipaplatform.paths import paths
+from ipapython.ipa_log_manager import root_logger

3.) the call to parent's __new__ should have signature
'super(aug_obj,
cls).__new__(*args, **kwargs)'

+cls._instance = super(aug_obj, cls).__new__(cls, *args,
**kwargs)

4.)

+raise RuntimeError('Augeas lenses was loaded. Could not
add more'
+   'lenses.')

Should be 'Augeas lenses _were_ loaded'

5.)

+if lens in self.lenses:
+raise RuntimeError('Lens %s already added.' % lens)
+self.lenses.append(lens)
+load_path = '/augeas/load/{0}'.format(lens


Shouldn't the following code use os.path,join to construct the
load_path?

6.) I would prefer the following indentation style in the add_lens()
method

@@ -65,9 +65,9 @@ class aug_obj(object):
 for conf_file in conf_files:
 self._aug.set(os.path.join(load_path, 'incl[0]'),
conf_file)
 self.tree['old'] = self.tree.get(conf_file, None)
-self.tree[conf_file] = aug_node(self._aug,
- os.path.join('/files',
- conf_file[1:]))
+self.tree[conf_file] = aug_node(
+self._aug, os.path.join('/files', conf_file[1:])
+)

7.) I would also prefer if the hardcoded paths like '/augeas/load',
'files', and '//error' would be made into either module variables or
class members.

8.)

+def load(self):
+if self._loaded:
+raise RuntimeError('Augeas lenses was loaded. Could not
add more'
+   'lenses.')

Fix the excpetion text in the same way as in 4.)

9.)

+errors = self._aug.match(os.path.join('//error'))

is the os.path.join necessary here?

10.) I guess you can rewrite the error message in load() method using
list comprehension:

@@ -76,9 +76,9 @@ class aug_obj(object):
 self._aug.load()
 errors = self._aug.match(os.path.join('//error'))
 if errors:
-err_msg = ""
-for error in errors:
-err_msg += ("{}: {}".format(error,
self._aug.get(error)))
+err_msg = '\n'.join(
+["{}: {}".format(e, self._aug.get(e)) for e in
errors]
+)
 raise RuntimeError(err_msg)
 self._loaded = True

11.)

+class aug_node(collections.MutableMapping):
+""" Single augeas node.
+Can be handled as python dict().
+"""
+def __init__(self, aug, path):
+self._aug = aug
+if path and os.path.isabs(path):
+self._path = path
+else:
+self._tmp = _tmp_path(aug, path)
+self._path = self._tmp.path

Isn't it better to change signature of __init__ to:

def __init__(self, aug, path=None):

and then test whether path is None?

12.)

def __setitem__(self, key, node):
+target = os.path.join(self._path, key)
+end = '{0}[0]'.format(os.path.join(self._path, key))
+if self._aug.match(target):
+self._aug.remove(target)
+target_list = aug_list(self._aug, target)
+for src_node in aug_list(node._aug, node._path):
+target_list.append(src_node)

The 'end' variable is declared but not used.

13.)

+
+def __len__(self):
+return len(self._aug.match('{0}/*'.format(self._path)))
+
+def __iter__(self):
+for key in self._aug.match('{0}/*'.format(self._path)):
+yield self._aug.label(key)
+raise StopIteration()
+

Shouldn't we construct paths using 

Re: [Freeipa-devel] [PATCH 0095-0098] NTP: use augeas, configure chronyd, do not overwrite config

[Martin Babinsky]

On 05/20/2016 02:29 PM, Martin Babinsky wrote:

On 05/16/2016 01:58 PM, David Kupka wrote:

On 26/04/16 10:09, David Kupka wrote:

On 14/03/16 14:01, Martin Basti wrote:



On 14.03.2016 13:46, Martin Babinsky wrote:

On 03/11/2016 09:16 AM, David Kupka wrote:

Current version (0.5.0) of python-augeas is missing copy() method.
Use
dkupka/python-augeas copr repo before new version it's build and
available in the official repos.




Hi David,

TLDR: NACK :D.

Here are high-level remarks to discuss:

Maybe it would be a good idea to move ipaaugeas/changeconf and ntp to
ipaplatform since it is dealing with (sorta) platform specific
behavior (ntp vs. chrony vs. whatever we will have for timesync in the
future). CC'ing Jan for thoughts.

Also regarding patches 0096-0097, we could have platform specific
TimeDateService object that could wrap NTP/chrony management. for
example, the task namespace functions in Pathc 0096 could be
reimplemented as a methods of the service (RedhatTimeDateService,
FedoraTimeDateService etc.) and then called in a platform-agnostic
manner.

Here are some comments regarding code:

Patch 0095:

1.)
+IPA_CUSTOM_AUGEAS_LENSES_DIR = '/usr/share/augeas/lenses/ipa/'

Do not forget to add this directory to %install and %files spection of
the spec file so that it is correctly added to RPM build.

2.)

please separate import of system-wide and IPA-specific modules by
blank line

+import collections
+from augeas import Augeas
+from ipaplatform.paths import paths
+from ipapython.ipa_log_manager import root_logger

3.) the call to parent's __new__ should have signature 'super(aug_obj,
cls).__new__(*args, **kwargs)'

+cls._instance = super(aug_obj, cls).__new__(cls, *args,
**kwargs)

4.)

+raise RuntimeError('Augeas lenses was loaded. Could not
add more'
+   'lenses.')

Should be 'Augeas lenses _were_ loaded'

5.)

+if lens in self.lenses:
+raise RuntimeError('Lens %s already added.' % lens)
+self.lenses.append(lens)
+load_path = '/augeas/load/{0}'.format(lens


Shouldn't the following code use os.path,join to construct the
load_path?

6.) I would prefer the following indentation style in the add_lens()
method

@@ -65,9 +65,9 @@ class aug_obj(object):
 for conf_file in conf_files:
 self._aug.set(os.path.join(load_path, 'incl[0]'),
conf_file)
 self.tree['old'] = self.tree.get(conf_file, None)
-self.tree[conf_file] = aug_node(self._aug,
- os.path.join('/files',
- conf_file[1:]))
+self.tree[conf_file] = aug_node(
+self._aug, os.path.join('/files', conf_file[1:])
+)

7.) I would also prefer if the hardcoded paths like '/augeas/load',
'files', and '//error' would be made into either module variables or
class members.

8.)

+def load(self):
+if self._loaded:
+raise RuntimeError('Augeas lenses was loaded. Could not
add more'
+   'lenses.')

Fix the excpetion text in the same way as in 4.)

9.)

+errors = self._aug.match(os.path.join('//error'))

is the os.path.join necessary here?

10.) I guess you can rewrite the error message in load() method using
list comprehension:

@@ -76,9 +76,9 @@ class aug_obj(object):
 self._aug.load()
 errors = self._aug.match(os.path.join('//error'))
 if errors:
-err_msg = ""
-for error in errors:
-err_msg += ("{}: {}".format(error,
self._aug.get(error)))
+err_msg = '\n'.join(
+["{}: {}".format(e, self._aug.get(e)) for e in
errors]
+)
 raise RuntimeError(err_msg)
 self._loaded = True

11.)

+class aug_node(collections.MutableMapping):
+""" Single augeas node.
+Can be handled as python dict().
+"""
+def __init__(self, aug, path):
+self._aug = aug
+if path and os.path.isabs(path):
+self._path = path
+else:
+self._tmp = _tmp_path(aug, path)
+self._path = self._tmp.path

Isn't it better to change signature of __init__ to:

def __init__(self, aug, path=None):

and then test whether path is None?

12.)

def __setitem__(self, key, node):
+target = os.path.join(self._path, key)
+end = '{0}[0]'.format(os.path.join(self._path, key))
+if self._aug.match(target):
+self._aug.remove(target)
+target_list = aug_list(self._aug, target)
+for src_node in aug_list(node._aug, node._path):
+target_list.append(src_node)

The 'end' variable is declared but not used.

13.)

+
+def __len__(self):
+return len(self._aug.match('{0}/*'.format(self._path)))
+
+def __iter__(self):
+for key in self._aug.match('{0}/*'.format(self._path)):
+yield self._aug.label(key)
+raise StopIteration()
+

Shouldn't we construct paths using os.path.join for the sake of
consistency?

Re: [Freeipa-devel] [PATCH 0095-0098] NTP: use augeas, configure chronyd, do not overwrite config

[Martin Babinsky]

On 05/16/2016 01:58 PM, David Kupka wrote:

On 26/04/16 10:09, David Kupka wrote:

On 14/03/16 14:01, Martin Basti wrote:



On 14.03.2016 13:46, Martin Babinsky wrote:

On 03/11/2016 09:16 AM, David Kupka wrote:

Current version (0.5.0) of python-augeas is missing copy() method. Use
dkupka/python-augeas copr repo before new version it's build and
available in the official repos.




Hi David,

TLDR: NACK :D.

Here are high-level remarks to discuss:

Maybe it would be a good idea to move ipaaugeas/changeconf and ntp to
ipaplatform since it is dealing with (sorta) platform specific
behavior (ntp vs. chrony vs. whatever we will have for timesync in the
future). CC'ing Jan for thoughts.

Also regarding patches 0096-0097, we could have platform specific
TimeDateService object that could wrap NTP/chrony management. for
example, the task namespace functions in Pathc 0096 could be
reimplemented as a methods of the service (RedhatTimeDateService,
FedoraTimeDateService etc.) and then called in a platform-agnostic
manner.

Here are some comments regarding code:

Patch 0095:

1.)
+IPA_CUSTOM_AUGEAS_LENSES_DIR = '/usr/share/augeas/lenses/ipa/'

Do not forget to add this directory to %install and %files spection of
the spec file so that it is correctly added to RPM build.

2.)

please separate import of system-wide and IPA-specific modules by
blank line

+import collections
+from augeas import Augeas
+from ipaplatform.paths import paths
+from ipapython.ipa_log_manager import root_logger

3.) the call to parent's __new__ should have signature 'super(aug_obj,
cls).__new__(*args, **kwargs)'

+cls._instance = super(aug_obj, cls).__new__(cls, *args,
**kwargs)

4.)

+raise RuntimeError('Augeas lenses was loaded. Could not
add more'
+   'lenses.')

Should be 'Augeas lenses _were_ loaded'

5.)

+if lens in self.lenses:
+raise RuntimeError('Lens %s already added.' % lens)
+self.lenses.append(lens)
+load_path = '/augeas/load/{0}'.format(lens


Shouldn't the following code use os.path,join to construct the
load_path?

6.) I would prefer the following indentation style in the add_lens()
method

@@ -65,9 +65,9 @@ class aug_obj(object):
 for conf_file in conf_files:
 self._aug.set(os.path.join(load_path, 'incl[0]'),
conf_file)
 self.tree['old'] = self.tree.get(conf_file, None)
-self.tree[conf_file] = aug_node(self._aug,
- os.path.join('/files',
- conf_file[1:]))
+self.tree[conf_file] = aug_node(
+self._aug, os.path.join('/files', conf_file[1:])
+)

7.) I would also prefer if the hardcoded paths like '/augeas/load',
'files', and '//error' would be made into either module variables or
class members.

8.)

+def load(self):
+if self._loaded:
+raise RuntimeError('Augeas lenses was loaded. Could not
add more'
+   'lenses.')

Fix the excpetion text in the same way as in 4.)

9.)

+errors = self._aug.match(os.path.join('//error'))

is the os.path.join necessary here?

10.) I guess you can rewrite the error message in load() method using
list comprehension:

@@ -76,9 +76,9 @@ class aug_obj(object):
 self._aug.load()
 errors = self._aug.match(os.path.join('//error'))
 if errors:
-err_msg = ""
-for error in errors:
-err_msg += ("{}: {}".format(error,
self._aug.get(error)))
+err_msg = '\n'.join(
+["{}: {}".format(e, self._aug.get(e)) for e in errors]
+)
 raise RuntimeError(err_msg)
 self._loaded = True

11.)

+class aug_node(collections.MutableMapping):
+""" Single augeas node.
+Can be handled as python dict().
+"""
+def __init__(self, aug, path):
+self._aug = aug
+if path and os.path.isabs(path):
+self._path = path
+else:
+self._tmp = _tmp_path(aug, path)
+self._path = self._tmp.path

Isn't it better to change signature of __init__ to:

def __init__(self, aug, path=None):

and then test whether path is None?

12.)

def __setitem__(self, key, node):
+target = os.path.join(self._path, key)
+end = '{0}[0]'.format(os.path.join(self._path, key))
+if self._aug.match(target):
+self._aug.remove(target)
+target_list = aug_list(self._aug, target)
+for src_node in aug_list(node._aug, node._path):
+target_list.append(src_node)

The 'end' variable is declared but not used.

13.)

+
+def __len__(self):
+return len(self._aug.match('{0}/*'.format(self._path)))
+
+def __iter__(self):
+for key in self._aug.match('{0}/*'.format(self._path)):
+yield self._aug.label(key)
+raise StopIteration()
+

Shouldn't we construct paths using os.path.join for the sake of
consistency?

14.)

+def __bool__(self):
+return 

Re: [Freeipa-devel] [PATCH 0095-0098] NTP: use augeas, configure chronyd, do not overwrite config

[Petr Spacek]

On 16.5.2016 13:58, David Kupka wrote:
>>>
>>>
>>> On 14.03.2016 13:46, Martin Babinsky wrote:
 On 03/11/2016 09:16 AM, David Kupka wrote:
> Current version (0.5.0) of python-augeas is missing copy() method. Use
> dkupka/python-augeas copr repo before new version it's build and
> available in the official repos.
>
>
>
 Hi David,

 TLDR: NACK :D.

 Here are high-level remarks to discuss:

 Maybe it would be a good idea to move ipaaugeas/changeconf and ntp to
 ipaplatform since it is dealing with (sorta) platform specific
 behavior (ntp vs. chrony vs. whatever we will have for timesync in the
 future). CC'ing Jan for thoughts.

 Also regarding patches 0096-0097, we could have platform specific
 TimeDateService object that could wrap NTP/chrony management. for
 example, the task namespace functions in Pathc 0096 could be
 reimplemented as a methods of the service (RedhatTimeDateService,
 FedoraTimeDateService etc.) and then called in a platform-agnostic
 manner.

 Here are some comments regarding code:

 Patch 0095:

 1.)
 +IPA_CUSTOM_AUGEAS_LENSES_DIR = '/usr/share/augeas/lenses/ipa/'

 Do not forget to add this directory to %install and %files spection of
 the spec file so that it is correctly added to RPM build.

 2.)

 please separate import of system-wide and IPA-specific modules by
 blank line

 +import collections
 +from augeas import Augeas
 +from ipaplatform.paths import paths
 +from ipapython.ipa_log_manager import root_logger

 3.) the call to parent's __new__ should have signature 'super(aug_obj,
 cls).__new__(*args, **kwargs)'

 +cls._instance = super(aug_obj, cls).__new__(cls, *args,
 **kwargs)

 4.)

 +raise RuntimeError('Augeas lenses was loaded. Could not
 add more'
 +   'lenses.')

 Should be 'Augeas lenses _were_ loaded'

 5.)

 +if lens in self.lenses:
 +raise RuntimeError('Lens %s already added.' % lens)
 +self.lenses.append(lens)
 +load_path = '/augeas/load/{0}'.format(lens


 Shouldn't the following code use os.path,join to construct the
 load_path?

 6.) I would prefer the following indentation style in the add_lens()
 method

 @@ -65,9 +65,9 @@ class aug_obj(object):
  for conf_file in conf_files:
  self._aug.set(os.path.join(load_path, 'incl[0]'),
 conf_file)
  self.tree['old'] = self.tree.get(conf_file, None)
 -self.tree[conf_file] = aug_node(self._aug,
 - os.path.join('/files',
 - conf_file[1:]))
 +self.tree[conf_file] = aug_node(
 +self._aug, os.path.join('/files', conf_file[1:])
 +)

 7.) I would also prefer if the hardcoded paths like '/augeas/load',
 'files', and '//error' would be made into either module variables or
 class members.

 8.)

 +def load(self):
 +if self._loaded:
 +raise RuntimeError('Augeas lenses was loaded. Could not
 add more'
 +   'lenses.')

 Fix the excpetion text in the same way as in 4.)

 9.)

 +errors = self._aug.match(os.path.join('//error'))

 is the os.path.join necessary here?

 10.) I guess you can rewrite the error message in load() method using
 list comprehension:

 @@ -76,9 +76,9 @@ class aug_obj(object):
  self._aug.load()
  errors = self._aug.match(os.path.join('//error'))
  if errors:
 -err_msg = ""
 -for error in errors:
 -err_msg += ("{}: {}".format(error,
 self._aug.get(error)))
 +err_msg = '\n'.join(
 +["{}: {}".format(e, self._aug.get(e)) for e in errors]
 +)
  raise RuntimeError(err_msg)
  self._loaded = True

 11.)

 +class aug_node(collections.MutableMapping):
 +""" Single augeas node.
 +Can be handled as python dict().
 +"""
 +def __init__(self, aug, path):
 +self._aug = aug
 +if path and os.path.isabs(path):
 +self._path = path
 +else:
 +self._tmp = _tmp_path(aug, path)
 +self._path = self._tmp.path

 Isn't it better to change signature of __init__ to:

 def __init__(self, aug, path=None):

 and then test whether path is None?

 12.)

 def __setitem__(self, key, node):
 +target = os.path.join(self._path, key)
 +end = '{0}[0]'.format(os.path.join(self._path, key))
 +if 

Re: [Freeipa-devel] [PATCH 0095-0098] NTP: use augeas, configure chronyd, do not overwrite config

[Petr Spacek]

On 26.4.2016 10:09, David Kupka wrote:
> diff --git a/ipapython/configfile.py b/ipapython/configfile.py
> new file mode 100644
> index 
> ..b48a9eae97dc4c1b19d6ae7e961ce701a4a36ed7
> --- /dev/null
> +++ b/ipapython/configfile.py

NACK, I think that Augeas magic is underdocumented. Possible form of
documentation are doctests :-)

-- 
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 0095-0098] NTP: use augeas, configure chronyd, do not overwrite config

[David Kupka]

On 14/03/16 14:01, Martin Basti wrote:



On 14.03.2016 13:46, Martin Babinsky wrote:

On 03/11/2016 09:16 AM, David Kupka wrote:

Current version (0.5.0) of python-augeas is missing copy() method. Use
dkupka/python-augeas copr repo before new version it's build and
available in the official repos.




Hi David,

TLDR: NACK :D.

Here are high-level remarks to discuss:

Maybe it would be a good idea to move ipaaugeas/changeconf and ntp to
ipaplatform since it is dealing with (sorta) platform specific
behavior (ntp vs. chrony vs. whatever we will have for timesync in the
future). CC'ing Jan for thoughts.

Also regarding patches 0096-0097, we could have platform specific
TimeDateService object that could wrap NTP/chrony management. for
example, the task namespace functions in Pathc 0096 could be
reimplemented as a methods of the service (RedhatTimeDateService,
FedoraTimeDateService etc.) and then called in a platform-agnostic
manner.

Here are some comments regarding code:

Patch 0095:

1.)
+IPA_CUSTOM_AUGEAS_LENSES_DIR = '/usr/share/augeas/lenses/ipa/'

Do not forget to add this directory to %install and %files spection of
the spec file so that it is correctly added to RPM build.

2.)

please separate import of system-wide and IPA-specific modules by
blank line

+import collections
+from augeas import Augeas
+from ipaplatform.paths import paths
+from ipapython.ipa_log_manager import root_logger

3.) the call to parent's __new__ should have signature 'super(aug_obj,
cls).__new__(*args, **kwargs)'

+cls._instance = super(aug_obj, cls).__new__(cls, *args,
**kwargs)

4.)

+raise RuntimeError('Augeas lenses was loaded. Could not
add more'
+   'lenses.')

Should be 'Augeas lenses _were_ loaded'

5.)

+if lens in self.lenses:
+raise RuntimeError('Lens %s already added.' % lens)
+self.lenses.append(lens)
+load_path = '/augeas/load/{0}'.format(lens


Shouldn't the following code use os.path,join to construct the load_path?

6.) I would prefer the following indentation style in the add_lens()
method

@@ -65,9 +65,9 @@ class aug_obj(object):
 for conf_file in conf_files:
 self._aug.set(os.path.join(load_path, 'incl[0]'), conf_file)
 self.tree['old'] = self.tree.get(conf_file, None)
-self.tree[conf_file] = aug_node(self._aug,
- os.path.join('/files',
- conf_file[1:]))
+self.tree[conf_file] = aug_node(
+self._aug, os.path.join('/files', conf_file[1:])
+)

7.) I would also prefer if the hardcoded paths like '/augeas/load',
'files', and '//error' would be made into either module variables or
class members.

8.)

+def load(self):
+if self._loaded:
+raise RuntimeError('Augeas lenses was loaded. Could not
add more'
+   'lenses.')

Fix the excpetion text in the same way as in 4.)

9.)

+errors = self._aug.match(os.path.join('//error'))

is the os.path.join necessary here?

10.) I guess you can rewrite the error message in load() method using
list comprehension:

@@ -76,9 +76,9 @@ class aug_obj(object):
 self._aug.load()
 errors = self._aug.match(os.path.join('//error'))
 if errors:
-err_msg = ""
-for error in errors:
-err_msg += ("{}: {}".format(error,
self._aug.get(error)))
+err_msg = '\n'.join(
+["{}: {}".format(e, self._aug.get(e)) for e in errors]
+)
 raise RuntimeError(err_msg)
 self._loaded = True

11.)

+class aug_node(collections.MutableMapping):
+""" Single augeas node.
+Can be handled as python dict().
+"""
+def __init__(self, aug, path):
+self._aug = aug
+if path and os.path.isabs(path):
+self._path = path
+else:
+self._tmp = _tmp_path(aug, path)
+self._path = self._tmp.path

Isn't it better to change signature of __init__ to:

def __init__(self, aug, path=None):

and then test whether path is None?

12.)

def __setitem__(self, key, node):
+target = os.path.join(self._path, key)
+end = '{0}[0]'.format(os.path.join(self._path, key))
+if self._aug.match(target):
+self._aug.remove(target)
+target_list = aug_list(self._aug, target)
+for src_node in aug_list(node._aug, node._path):
+target_list.append(src_node)

The 'end' variable is declared but not used.

13.)

+
+def __len__(self):
+return len(self._aug.match('{0}/*'.format(self._path)))
+
+def __iter__(self):
+for key in self._aug.match('{0}/*'.format(self._path)):
+yield self._aug.label(key)
+raise StopIteration()
+

Shouldn't we construct paths using os.path.join for the sake of
consistency?

14.)

+def __bool__(self):
+return (bool(len(self)) or bool(self.value))

The parentheses around the boolean expression 

Re: [Freeipa-devel] [PATCH 0095-0098] NTP: use augeas, configure chronyd, do not overwrite config

[Martin Basti]

On 14.03.2016 13:46, Martin Babinsky wrote:

On 03/11/2016 09:16 AM, David Kupka wrote:

Current version (0.5.0) of python-augeas is missing copy() method. Use
dkupka/python-augeas copr repo before new version it's build and
available in the official repos.




Hi David,

TLDR: NACK :D.

Here are high-level remarks to discuss:

Maybe it would be a good idea to move ipaaugeas/changeconf and ntp to 
ipaplatform since it is dealing with (sorta) platform specific 
behavior (ntp vs. chrony vs. whatever we will have for timesync in the 
future). CC'ing Jan for thoughts.


Also regarding patches 0096-0097, we could have platform specific 
TimeDateService object that could wrap NTP/chrony management. for 
example, the task namespace functions in Pathc 0096 could be 
reimplemented as a methods of the service (RedhatTimeDateService, 
FedoraTimeDateService etc.) and then called in a platform-agnostic 
manner.


Here are some comments regarding code:

Patch 0095:

1.)
+IPA_CUSTOM_AUGEAS_LENSES_DIR = '/usr/share/augeas/lenses/ipa/'

Do not forget to add this directory to %install and %files spection of 
the spec file so that it is correctly added to RPM build.


2.)

please separate import of system-wide and IPA-specific modules by 
blank line


+import collections
+from augeas import Augeas
+from ipaplatform.paths import paths
+from ipapython.ipa_log_manager import root_logger

3.) the call to parent's __new__ should have signature 'super(aug_obj, 
cls).__new__(*args, **kwargs)'


+cls._instance = super(aug_obj, cls).__new__(cls, *args, 
**kwargs)


4.)

+raise RuntimeError('Augeas lenses was loaded. Could not 
add more'

+   'lenses.')

Should be 'Augeas lenses _were_ loaded'

5.)

+if lens in self.lenses:
+raise RuntimeError('Lens %s already added.' % lens)
+self.lenses.append(lens)
+load_path = '/augeas/load/{0}'.format(lens


Shouldn't the following code use os.path,join to construct the load_path?

6.) I would prefer the following indentation style in the add_lens() 
method


@@ -65,9 +65,9 @@ class aug_obj(object):
 for conf_file in conf_files:
 self._aug.set(os.path.join(load_path, 'incl[0]'), conf_file)
 self.tree['old'] = self.tree.get(conf_file, None)
-self.tree[conf_file] = aug_node(self._aug,
- os.path.join('/files',
- conf_file[1:]))
+self.tree[conf_file] = aug_node(
+self._aug, os.path.join('/files', conf_file[1:])
+)

7.) I would also prefer if the hardcoded paths like '/augeas/load', 
'files', and '//error' would be made into either module variables or 
class members.


8.)

+def load(self):
+if self._loaded:
+raise RuntimeError('Augeas lenses was loaded. Could not 
add more'

+   'lenses.')

Fix the excpetion text in the same way as in 4.)

9.)

+errors = self._aug.match(os.path.join('//error'))

is the os.path.join necessary here?

10.) I guess you can rewrite the error message in load() method using 
list comprehension:


@@ -76,9 +76,9 @@ class aug_obj(object):
 self._aug.load()
 errors = self._aug.match(os.path.join('//error'))
 if errors:
-err_msg = ""
-for error in errors:
-err_msg += ("{}: {}".format(error, 
self._aug.get(error)))

+err_msg = '\n'.join(
+["{}: {}".format(e, self._aug.get(e)) for e in errors]
+)
 raise RuntimeError(err_msg)
 self._loaded = True

11.)

+class aug_node(collections.MutableMapping):
+""" Single augeas node.
+Can be handled as python dict().
+"""
+def __init__(self, aug, path):
+self._aug = aug
+if path and os.path.isabs(path):
+self._path = path
+else:
+self._tmp = _tmp_path(aug, path)
+self._path = self._tmp.path

Isn't it better to change signature of __init__ to:

def __init__(self, aug, path=None):

and then test whether path is None?

12.)

def __setitem__(self, key, node):
+target = os.path.join(self._path, key)
+end = '{0}[0]'.format(os.path.join(self._path, key))
+if self._aug.match(target):
+self._aug.remove(target)
+target_list = aug_list(self._aug, target)
+for src_node in aug_list(node._aug, node._path):
+target_list.append(src_node)

The 'end' variable is declared but not used.

13.)

+
+def __len__(self):
+return len(self._aug.match('{0}/*'.format(self._path)))
+
+def __iter__(self):
+for key in self._aug.match('{0}/*'.format(self._path)):
+yield self._aug.label(key)
+raise StopIteration()
+

Shouldn't we construct paths using os.path.join for the sake of 
consistency?


14.)

+def __bool__(self):
+return (bool(len(self)) or bool(self.value))

The parentheses around the boolean expression are 

Re: [Freeipa-devel] [PATCH 0095-0098] NTP: use augeas, configure chronyd, do not overwrite config

[Martin Babinsky]

On 03/11/2016 09:16 AM, David Kupka wrote:

Current version (0.5.0) of python-augeas is missing copy() method. Use
dkupka/python-augeas copr repo before new version it's build and
available in the official repos.




Hi David,

TLDR: NACK :D.

Here are high-level remarks to discuss:

Maybe it would be a good idea to move ipaaugeas/changeconf and ntp to 
ipaplatform since it is dealing with (sorta) platform specific behavior 
(ntp vs. chrony vs. whatever we will have for timesync in the future). 
CC'ing Jan for thoughts.


Also regarding patches 0096-0097, we could have platform specific 
TimeDateService object that could wrap NTP/chrony management. for 
example, the task namespace functions in Pathc 0096 could be 
reimplemented as a methods of the service (RedhatTimeDateService, 
FedoraTimeDateService etc.) and then called in a platform-agnostic manner.


Here are some comments regarding code:

Patch 0095:

1.)
+IPA_CUSTOM_AUGEAS_LENSES_DIR = '/usr/share/augeas/lenses/ipa/'

Do not forget to add this directory to %install and %files spection of 
the spec file so that it is correctly added to RPM build.


2.)

please separate import of system-wide and IPA-specific modules by blank line

+import collections
+from augeas import Augeas
+from ipaplatform.paths import paths
+from ipapython.ipa_log_manager import root_logger

3.) the call to parent's __new__ should have signature 'super(aug_obj, 
cls).__new__(*args, **kwargs)'


+cls._instance = super(aug_obj, cls).__new__(cls, *args, 
**kwargs)


4.)

+raise RuntimeError('Augeas lenses was loaded. Could not add 
more'

+   'lenses.')

Should be 'Augeas lenses _were_ loaded'

5.)

+if lens in self.lenses:
+raise RuntimeError('Lens %s already added.' % lens)
+self.lenses.append(lens)
+load_path = '/augeas/load/{0}'.format(lens


Shouldn't the following code use os.path,join to construct the load_path?

6.) I would prefer the following indentation style in the add_lens() method

@@ -65,9 +65,9 @@ class aug_obj(object):
 for conf_file in conf_files:
 self._aug.set(os.path.join(load_path, 'incl[0]'), conf_file)
 self.tree['old'] = self.tree.get(conf_file, None)
-self.tree[conf_file] = aug_node(self._aug,
-os.path.join('/files',
- conf_file[1:]))
+self.tree[conf_file] = aug_node(
+self._aug, os.path.join('/files', conf_file[1:])
+)

7.) I would also prefer if the hardcoded paths like '/augeas/load', 
'files', and '//error' would be made into either module variables or 
class members.


8.)

+def load(self):
+if self._loaded:
+raise RuntimeError('Augeas lenses was loaded. Could not add 
more'

+   'lenses.')

Fix the excpetion text in the same way as in 4.)

9.)

+errors = self._aug.match(os.path.join('//error'))

is the os.path.join necessary here?

10.) I guess you can rewrite the error message in load() method using 
list comprehension:


@@ -76,9 +76,9 @@ class aug_obj(object):
 self._aug.load()
 errors = self._aug.match(os.path.join('//error'))
 if errors:
-err_msg = ""
-for error in errors:
-err_msg += ("{}: {}".format(error, self._aug.get(error)))
+err_msg = '\n'.join(
+["{}: {}".format(e, self._aug.get(e)) for e in errors]
+)
 raise RuntimeError(err_msg)
 self._loaded = True

11.)

+class aug_node(collections.MutableMapping):
+""" Single augeas node.
+Can be handled as python dict().
+"""
+def __init__(self, aug, path):
+self._aug = aug
+if path and os.path.isabs(path):
+self._path = path
+else:
+self._tmp = _tmp_path(aug, path)
+self._path = self._tmp.path

Isn't it better to change signature of __init__ to:

def __init__(self, aug, path=None):

and then test whether path is None?

12.)

def __setitem__(self, key, node):
+target = os.path.join(self._path, key)
+end = '{0}[0]'.format(os.path.join(self._path, key))
+if self._aug.match(target):
+self._aug.remove(target)
+target_list = aug_list(self._aug, target)
+for src_node in aug_list(node._aug, node._path):
+target_list.append(src_node)

The 'end' variable is declared but not used.

13.)

+
+def __len__(self):
+return len(self._aug.match('{0}/*'.format(self._path)))
+
+def __iter__(self):
+for key in self._aug.match('{0}/*'.format(self._path)):
+yield self._aug.label(key)
+raise StopIteration()
+

Shouldn't we construct paths using os.path.join for the sake of consistency?

14.)

+def __bool__(self):
+return (bool(len(self)) or bool(self.value))

The 

[Freeipa-devel] [PATCH 0095-0098] NTP: use augeas, configure chronyd, do not overwrite config

[David Kupka]

Current version (0.5.0) of python-augeas is missing copy() method. Use 
dkupka/python-augeas copr repo before new version it's build and 
available in the official repos.


--
David Kupka

From 5f9419f9a0e56119fc80f4cf1f7c2ffde15268a0 Mon Sep 17 00:00:00 2001
From: David Kupka 
Date: Thu, 10 Mar 2016 22:33:15 +0100
Subject: [PATCH 1/4] augeas: add wrapper around python binding

python-augeas binding is mostly 1-1 mapping of C functions. Put a layer of
list- and dict-like classes to allow a slightly more comfortable work.

https://fedorahosted.org/freeipa/ticket/4920
---
 freeipa.spec.in   |   1 +
 ipaplatform/base/paths.py |   1 +
 ipapython/ipaaugeas.py| 269 ++
 3 files changed, 271 insertions(+)
 create mode 100644 ipapython/ipaaugeas.py

diff --git a/freeipa.spec.in b/freeipa.spec.in
index 9e277020d70215e052ab6c905b1c6a29ae6cdd4d..a7cc91cec3aa660bc76b7873723d5f3241cdf1d9 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -171,6 +171,7 @@ Requires: systemd-python
 Requires: %{etc_systemd_dir}
 Requires: gzip
 Requires: oddjob
+Requires: python-augeas > 0.5.0
 
 Provides: %{alt_name}-server = %{version}
 Conflicts: %{alt_name}-server
diff --git a/ipaplatform/base/paths.py b/ipaplatform/base/paths.py
index bdff4f3934f3250bdfef3f913631b98d55d759b6..c0200122246e83edab2905ecf6c353b928ce88cf 100644
--- a/ipaplatform/base/paths.py
+++ b/ipaplatform/base/paths.py
@@ -345,5 +345,6 @@ class BasePathNamespace(object):
 IPA_CUSTODIA_SOCKET = '/run/httpd/ipa-custodia.sock'
 IPA_CUSTODIA_AUDIT_LOG = '/var/log/ipa-custodia.audit.log'
 IPA_GETKEYTAB = '/usr/sbin/ipa-getkeytab'
+IPA_CUSTOM_AUGEAS_LENSES_DIR = '/usr/share/augeas/lenses/ipa/'
 
 path_namespace = BasePathNamespace
diff --git a/ipapython/ipaaugeas.py b/ipapython/ipaaugeas.py
new file mode 100644
index ..1e67ada447467c08e73e55c1576ac33233e4b64f
--- /dev/null
+++ b/ipapython/ipaaugeas.py
@@ -0,0 +1,269 @@
+#!/usr/bin/python
+
+import os
+import shutil
+import tempfile
+import collections
+from augeas import Augeas
+from ipaplatform.paths import paths
+from ipapython.ipa_log_manager import root_logger
+
+
+class _tmp_path(object):
+"""Create a unique temporary path.
+
+Uniquenes depends on tempfile.mkdtemp ability to create unique directory
+path.
+The path is cleared when the object is deleted.
+"""
+def __init__(self, aug, label):
+self._aug = aug
+self._tmp = tempfile.mkdtemp()
+if label:
+self.path = os.path.join(self._tmp, label)
+else:
+self.path = self._tmp
+
+def __del__(self):
+self._aug.remove(self._tmp)
+shutil.rmtree(self._tmp)
+
+
+class aug_obj(object):
+"""python-augeas higher-level wrapper.
+Load single augeas lens and related configuration file.
+Allows working with augeas tree like dict(key:list(dict(...), ...), ...))
+
+Can be used in with statement in the same way as file does.
+"""
+
+_instance = None
+_loaded = False
+
+def __new__(cls, *args, **kwargs):
+if not cls._instance:
+cls._instance = super(aug_obj, cls).__new__(cls, *args, **kwargs)
+return cls._instance
+
+def __init__(self):
+if not hasattr(self, '_aug'):
+self._aug = Augeas(loadpath=paths.IPA_CUSTOM_AUGEAS_LENSES_DIR,
+   flags=Augeas.NO_MODL_AUTOLOAD | Augeas.NO_LOAD)
+self.lenses = []
+self.tree = {}
+
+def add_lens(self, lens, conf_files):
+if self._loaded:
+raise RuntimeError('Augeas lenses was loaded. Could not add more'
+   'lenses.')
+if lens in self.lenses:
+raise RuntimeError('Lens %s already added.' % lens)
+self.lenses.append(lens)
+load_path = '/augeas/load/{0}'.format(lens)
+
+self._aug.set(os.path.join(load_path, 'lens'), lens)
+for conf_file in conf_files:
+self._aug.set(os.path.join(load_path, 'incl[0]'), conf_file)
+self.tree['old'] = self.tree.get(conf_file, None)
+self.tree[conf_file] = aug_node(self._aug,
+os.path.join('/files',
+ conf_file[1:]))
+
+def load(self):
+if self._loaded:
+raise RuntimeError('Augeas lenses was loaded. Could not add more'
+   'lenses.')
+self._aug.load()
+errors = self._aug.match(os.path.join('//error'))
+if errors:
+err_msg = ""
+for error in errors:
+err_msg += ("{}: {}".format(error, self._aug.get(error)))
+raise RuntimeError(err_msg)
+self._loaded = True
+
+def __enter__(self):
+return self
+
+def __exit__(self, exc_type, exc_value, traceback):
+self.flush()
+