Alon Bar-Lev has posted comments on this change.

Change subject: gluster: Added a plugin under gluster for nrpe configuration.
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.ovirt.org/#/c/26956/3/src/plugins/ovirt-host-deploy/gluster/nrpe.py
File src/plugins/ovirt-host-deploy/gluster/nrpe.py:

Line 66:         for index, line in enumerate(content):
Line 67:             if line.startswith('allowed_hosts') and line.find(
Line 68:                     self.environment[
Line 69:                         odeploycons.GlusterEnv.MONITORING_SERVER
Line 70:                     ]) == -1:
please close ')' in the start of indent, this makes the indent obvious.

please put each boolean expression in own line when have complex expression.

in this case:

 if (
     line.startswith('allowed_hosts') and
     line.find(
         self.environment[
     ) == -1
 ):

but anyway... this is no good...

as you will find 10.1.1.100 when you have 10.1.1.1 in environment.
Line 71:                 if line.endswith('allowed_hosts='):
Line 72:                     content[index] = line + self.environment[
Line 73:                         odeploycons.GlusterEnv.MONITORING_SERVER
Line 74:                     ]


Line 74:                     ]
Line 75:                 else:
Line 76:                     content[index] = line + ',' + self.environment[
Line 77:                         odeploycons.GlusterEnv.MONITORING_SERVER
Line 78:                     ]
I suggest a different approach... more complete... using python abilities.

please see other examples within sources wew to put regular expression 
template. please update file only if actually modified.

 import re
 
 _RE_KEY_VALUE = re.compile(
     flags=re.VERBOSE,
     pattern=r"""
         ^
         \s*
         (?P<key>\w+)
         \s*
         =
         \s*
         (?P<value>[^#]*)
         ([#].*)?
         $
     """
 )
 
 content="""
 # comment
 key1=value1
 key2=value2 # comment
 key3=
 key4= #comment
 #key5=value5
 allowed_hosts=
 allowed_hosts=a,b,c
 allowed_hosts=d,a,b,c
 """.splitlines()
 
 modified = False
 newcontent = []
 for l in content:
     m = _RE_KEY_VALUE.match(l)
     if m is not None and m.group('key') == 'allowed_hosts':
         s = set([e.strip() for e in m.group('value').split(',') if e.strip()])
         if 'd' not in s:
             s.add('d')
             l = 'allowed_hosts=%s' % ','.join(sorted(s))
     newcontent.append(l)
 
 if newcontent != content:
     # transaction with newcontent
     print('\n'.join(newcontent))
Line 79:         self.environment[
Line 80:             otopicons.CoreEnv.MAIN_TRANSACTION
Line 81:         ].append(
Line 82:             filetransaction.FileTransaction(


-- 
To view, visit http://gerrit.ovirt.org/26956
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4faa40e2bf382187907db944f7485c1630fcc0ae
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-host-deploy
Gerrit-Branch: master
Gerrit-Owner: Darshan N <dnara...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Darshan N <dnara...@redhat.com>
Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com>
Gerrit-Reviewer: Sahina Bose <sab...@redhat.com>
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to