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