On 10/02/2012 10:49 AM, Petr Viktorin wrote: > On 09/27/2012 01:35 PM, Martin Kosek wrote: >> A hotfix pushed in a scope of ticket 3088 forced conversion of DN >> object (baseDN) in IPA client discovery so that ipa-client-install >> does not crash when creating an IPA default.conf. Since this is not >> a preferred way to handle DN objects, improve its usage: >> >> - make sure, that baseDN retrieved by client discovery is always >> a DN object >> - update ipachangeconf.py code to handle strings better and instead >> of concatenating objects, make sure they are converted to string >> first >> >> As a side-effect of ipachangeconf changes, default.conf config file >> generated by ipa-client-install has no longer empty new line at the >> end of a file. >> >> https://fedorahosted.org/freeipa/ticket/3088 > > ACK, but since you're reformatting the code, here are some minor issues found > by the PEP8 checker. > >> diff --git a/ipa-client/ipaclient/ipachangeconf.py >> b/ipa-client/ipaclient/ipachangeconf.py >> index >> f6288062be5c5d1b29341ed90814e1fa1431019c..2bb0cc487670ce74833fc09b06449d0a176ffaf5 >> 100644 >> --- a/ipa-client/ipaclient/ipachangeconf.py >> +++ b/ipa-client/ipaclient/ipachangeconf.py >> @@ -68,7 +68,7 @@ class IPAChangeConf: >> elif type(indent) is str: >> self.indent = (indent, ) >> else: >> - raise ValueError, 'Indent must be a list of strings' >> + raise ValueError('Indent must be a list of strings') > > Please re-indent to 4 spaces. > >> >> @@ -143,35 +143,50 @@ class IPAChangeConf: >> def getSectionLine(self, section): >> if len(self.sectnamdel) != 2: >> return section >> - return self.sectnamdel[0]+section+self.sectnamdel[1]+self.deol >> + return self._dump_line(self.sectnamdel[0], >> + section, >> + self.sectnamdel[1], >> + self.deol) >> + >> + def _dump_line(self, *args): >> + return u"".join(unicode(x) for x in args) >> >> def dump(self, options, level=0): >> - output = "" >> + output = [] >> if level >= len(self.indent): >> level = len(self.indent)-1 >> >> for o in options: >> if o['type'] == "section": >> - output += >> self.sectnamdel[0]+o['name']+self.sectnamdel[1]+self.deol >> - output += self.dump(o['value'], level+1) >> + output.append(self._dump_line(self.sectnamdel[0], >> + o['name'], >> + self.sectnamdel[1])) >> + output.append(self.dump(o['value'], level+1)) > > Please surround the + operator by spaces (here & below). > >> @@ -274,15 +297,19 @@ class IPAChangeConf: >> opts.append(o) >> continue >> if no['action'] == 'comment': >> - opts.append({'name':'comment', 'type':'comment', >> - >> 'value':self.dcomment+o['name']+self.dassign+o['value']}) >> + value = self._dump_line(self.dcomment, >> + o['name'], >> + self.dassign, >> + o['value']) >> + opts.append({'name':'comment', 'type':'comment', >> + 'value':value}) > > Please add a space after each colon. >
Since the whole ipachangeconf.py is not much PEP8 compliant, I did not want to create just small "islands of compliance" with different coding style that the rest of the file. In attached patch I fixed the PEP8 warnings in the whole file. Martin
From 35ed5e11024981dae2b49ef785b2067af20ace93 Mon Sep 17 00:00:00 2001 From: Martin Kosek <mko...@redhat.com> Date: Thu, 27 Sep 2012 12:40:55 +0200 Subject: [PATCH] Improve DN usage in ipa-client-install A hotfix pushed in a scope of ticket 3088 forced conversion of DN object (baseDN) in IPA client discovery so that ipa-client-install does not crash when creating an IPA default.conf. Since this is not a preferred way to handle DN objects, improve its usage: - make sure, that baseDN retrieved by client discovery is always a DN object - update ipachangeconf.py code to handle strings better and instead of concatenating objects, make sure they are converted to string first As a side-effect of ipachangeconf changes, default.conf config file generated by ipa-client-install has no longer empty new line at the end of a file. Whole ipachangeconf.py has been modified to be compliant with PEP8. https://fedorahosted.org/freeipa/ticket/3088 --- ipa-client/ipaclient/ipachangeconf.py | 179 ++++++++++++++++++++++------------ ipa-client/ipaclient/ipadiscovery.py | 2 +- ipapython/ipautil.py | 2 +- 3 files changed, 119 insertions(+), 64 deletions(-) diff --git a/ipa-client/ipaclient/ipachangeconf.py b/ipa-client/ipaclient/ipachangeconf.py index f6288062be5c5d1b29341ed90814e1fa1431019c..6cf47b807957c245fe03ff4259e35526c49175a9 100644 --- a/ipa-client/ipaclient/ipachangeconf.py +++ b/ipa-client/ipaclient/ipachangeconf.py @@ -24,11 +24,12 @@ import string import time import shutil + def openLocked(filename, perms): fd = -1 try: fd = os.open(filename, os.O_RDWR | os.O_CREAT, perms) - + fcntl.lockf(fd, fcntl.LOCK_EX) except OSError, (errno, strerr): if fd != -1: @@ -49,15 +50,15 @@ class IPAChangeConf: def __init__(self, name): self.progname = name - self.indent = ("","","") - self.assign = (" = ","=") + self.indent = ("", "", "") + self.assign = (" = ", "=") self.dassign = self.assign[0] self.comment = ("#",) self.dcomment = self.comment[0] self.eol = ("\n",) self.deol = self.eol[0] - self.sectnamdel = ("[","]") - self.subsectdel = ("{","}") + self.sectnamdel = ("[", "]") + self.subsectdel = ("{", "}") def setProgName(self, name): self.progname = name @@ -68,7 +69,7 @@ class IPAChangeConf: elif type(indent) is str: self.indent = (indent, ) else: - raise ValueError, 'Indent must be a list of strings' + raise ValueError('Indent must be a list of strings') def setOptionAssignment(self, assign): if type(assign) is tuple: @@ -116,7 +117,7 @@ class IPAChangeConf: return False if not cl.endswith(self.sectnamdel[1]): return False - return cl[len(self.sectnamdel[0]):-len(self.sectnamdel[1])] + return cl[len(self.sectnamdel[0]):-len(self.sectnamdel[1])] def matchSubSection(self, line): if self.matchComment(line): @@ -143,50 +144,69 @@ class IPAChangeConf: def getSectionLine(self, section): if len(self.sectnamdel) != 2: return section - return self.sectnamdel[0]+section+self.sectnamdel[1]+self.deol + return self._dump_line(self.sectnamdel[0], + section, + self.sectnamdel[1], + self.deol) + + def _dump_line(self, *args): + return u"".join(unicode(x) for x in args) def dump(self, options, level=0): - output = "" + output = [] if level >= len(self.indent): - level = len(self.indent)-1 + level = len(self.indent) - 1 for o in options: if o['type'] == "section": - output += self.sectnamdel[0]+o['name']+self.sectnamdel[1]+self.deol - output += self.dump(o['value'], level+1) + output.append(self._dump_line(self.sectnamdel[0], + o['name'], + self.sectnamdel[1])) + output.append(self.dump(o['value'], (level + 1))) continue if o['type'] == "subsection": - output += self.indent[level]+o['name']+self.dassign+self.subsectdel[0]+self.deol - output += self.dump(o['value'], level+1) - output += self.indent[level]+self.subsectdel[1]+self.deol + output.append(self._dump_line(self.indent[level], + o['name'], + self.dassign, + self.subsectdel[0])) + output.append(self.dump(o['value'], (level + 1))) + output.append(self._dump_line(self.indent[level], + self.subsectdel[1])) continue if o['type'] == "option": - output += self.indent[level]+o['name']+self.dassign+o['value']+self.deol + output.append(self._dump_line(self.indent[level], + o['name'], + self.dassign, + o['value'])) continue if o['type'] == "comment": - output += self.dcomment+o['value']+self.deol + output.append(self._dump_line(self.dcomment, o['value'])) continue if o['type'] == "empty": - output += self.deol + output.append('') continue - raise SyntaxError, 'Unknown type: ['+o['type']+']' + raise SyntaxError('Unknown type: [%s]' % o['type']) - return output + return self.deol.join(output) def parseLine(self, line): if self.matchEmpty(line): - return {'name':'empty', 'type':'empty'} + return {'name': 'empty', 'type': 'empty'} value = self.matchComment(line) if value: - return {'name':'comment', 'type':'comment', 'value':value.rstrip()} #pylint: disable=E1103 + return {'name': 'comment', + 'type': 'comment', + 'value': value.rstrip()} # pylint: disable=E1103 parts = line.split(self.dassign, 1) if len(parts) < 2: - raise SyntaxError, 'Syntax Error: Unknown line format' + raise SyntaxError('Syntax Error: Unknown line format') - return {'name':parts[0].strip(), 'type':'option', 'value':parts[1].rstrip()} + return {'name': parts[0].strip(), + 'type': 'option', + 'value': parts[1].rstrip()} def findOpts(self, opts, type, name, exclude_sections=False): @@ -194,46 +214,65 @@ class IPAChangeConf: for o in opts: if o['type'] == type and o['name'] == name: return (num, o) - if exclude_sections and (o['type'] == "section" or o['type'] == "subsection"): + if exclude_sections and (o['type'] == "section" or + o['type'] == "subsection"): return (num, None) num += 1 return (num, None) - def commentOpts(self, inopts, level = 0): + def commentOpts(self, inopts, level=0): opts = [] if level >= len(self.indent): - level = len(self.indent)-1 + level = len(self.indent) - 1 for o in inopts: if o['type'] == 'section': - no = self.commentOpts(o['value'], level+1) - val = self.dcomment+self.sectnamdel[0]+o['name']+self.sectnamdel[1] - opts.append({'name':'comment', 'type':'comment', 'value':val}) + no = self.commentOpts(o['value'], (level + 1)) + val = self._dump_line(self.dcomment, + self.sectnamdel[0], + o['name'], + self.sectnamdel[1]) + opts.append({'name': 'comment', + 'type': 'comment', + 'value': val}) for n in no: opts.append(n) continue if o['type'] == 'subsection': - no = self.commentOpts(o['value'], level+1) - val = self.indent[level]+o['name']+self.dassign+self.subsectdel[0] - opts.append({'name':'comment', 'type':'comment', 'value':val}) - for n in no: - opts.append(n) - val = self.indent[level]+self.subsectdel[1] - opts.append({'name':'comment', 'type':'comment', 'value':val}) + no = self.commentOpts(o['value'], (level + 1)) + val = self._dump_line(self.indent[level], + o['name'], + self.dassign, + self.subsectdel[0]) + opts.append({'name': 'comment', + 'type': 'comment', + 'value': val}) + opts.extend(no) + val = self._dump_line(self.indent[level], self.subsectdel[1]) + opts.append({'name': 'comment', + 'type': 'comment', + 'value': val}) continue if o['type'] == 'option': - val = self.indent[level]+o['name']+self.dassign+o['value'] - opts.append({'name':'comment', 'type':'comment', 'value':val}) + val = self._dump_line(self.indent[level], + o['name'], + self.dassign, + o['value']) + opts.append({'name': 'comment', + 'type': 'comment', + 'value': val}) continue if o['type'] == 'comment': opts.append(o) continue if o['type'] == 'empty': - opts.append({'name':'comment', 'type':'comment', 'value':''}) + opts.append({'name': 'comment', + 'type': 'comment', + 'value': ''}) continue - raise SyntaxError, 'Unknown type: ['+o['type']+']' + raise SyntaxError('Unknown type: [%s]' % o['type']) return opts @@ -249,7 +288,9 @@ class IPAChangeConf: continue if no['action'] == "set": mo = self.mergeOld(o['value'], no['value']) - opts.append({'name':o['name'], 'type':o['type'], 'value':mo}) + opts.append({'name': o['name'], + 'type': o['type'], + 'value': mo}) continue if no['action'] == "comment": co = self.commentOpts(o['value']) @@ -258,11 +299,11 @@ class IPAChangeConf: continue if no['action'] == "remove": continue - raise SyntaxError, 'Unknown action: ['+no['action']+']' + raise SyntaxError('Unknown action: [%s]' % no['action']) if o['type'] == "comment" or o['type'] == "empty": - opts.append(o) - continue + opts.append(o) + continue if o['type'] == "option": (num, no) = self.findOpts(newopts, 'option', o['name'], True) @@ -270,19 +311,25 @@ class IPAChangeConf: opts.append(o) continue if no['action'] == 'comment' or no['action'] == 'remove': - if no['value'] != None and o['value'] != no['value']: + if (no['value'] is not None and + o['value'] is not no['value']): opts.append(o) continue if no['action'] == 'comment': - opts.append({'name':'comment', 'type':'comment', - 'value':self.dcomment+o['name']+self.dassign+o['value']}) + value = self._dump_line(self.dcomment, + o['name'], + self.dassign, + o['value']) + opts.append({'name': 'comment', + 'type': 'comment', + 'value': value}) continue if no['action'] == 'set': opts.append(no) continue - raise SyntaxError, 'Unknown action: ['+o['action']+']' + raise SyntaxError('Unknown action: [%s]' % o['action']) - raise SyntaxError, 'Unknown type: ['+o['type']+']' + raise SyntaxError('Unknown type: [%s]' % o['type']) return opts @@ -301,7 +348,7 @@ class IPAChangeConf: if no['action'] == "set": self.mergeNew(o['value'], no['value']) continue - cline = num+1 + cline = num + 1 continue if no['type'] == "option": @@ -310,7 +357,7 @@ class IPAChangeConf: if no['action'] == 'set': opts.append(no) continue - cline = num+1 + cline = num + 1 continue if no['type'] == "comment" or no['type'] == "empty": @@ -318,8 +365,7 @@ class IPAChangeConf: cline += 1 continue - raise SyntaxError, 'Unknown type: ['+no['type']+']' - + raise SyntaxError('Unknown type: [%s]' % no['type']) def merge(self, oldopts, newopts): @@ -352,7 +398,9 @@ class IPAChangeConf: value = self.matchSection(line) if value: if section is not None: - opts.append({'name':section, 'type':'section', 'value':sectopts}) + opts.append({'name': section, + 'type': 'section', + 'value': sectopts}) sectopts = [] curopts = sectopts fatheropts = sectopts @@ -363,7 +411,8 @@ class IPAChangeConf: value = self.matchSubSection(line) if value: if subsection is not None: - raise SyntaxError, 'nested subsections are not supported yet' + raise SyntaxError('nested subsections are not ' + 'supported yet') subsectopts = [] curopts = subsectopts subsection = value @@ -372,8 +421,11 @@ class IPAChangeConf: value = self.matchSubSectionEnd(line) if value: if subsection is None: - raise SyntaxError, 'Unmatched end subsection terminator found' - fatheropts.append({'name':subsection, 'type':'subsection', 'value':subsectopts}) + raise SyntaxError('Unmatched end subsection terminator ' + 'found') + fatheropts.append({'name': subsection, + 'type': 'subsection', + 'value': subsectopts}) subsection = None curopts = fatheropts continue @@ -383,7 +435,9 @@ class IPAChangeConf: #Add last section if any if len(sectopts) is not 0: - opts.append({'name':section, 'type':'section', 'value':sectopts}) + opts.append({'name': section, + 'type': 'section', + 'value': sectopts}) return opts @@ -399,8 +453,9 @@ class IPAChangeConf: output = "" f = None try: - #Do not catch an unexisting file error, we want to fail in that case - shutil.copy2(file, file+".ipabkp") + # Do not catch an unexisting file error + # we want to fail in that case + shutil.copy2(file, (file + ".ipabkp")) f = openLocked(file, 0644) @@ -435,7 +490,7 @@ class IPAChangeConf: f = None try: try: - shutil.copy2(file, file+".ipabkp") + shutil.copy2(file, (file + ".ipabkp")) except IOError, err: if err.errno == 2: # The orign file did not exist diff --git a/ipa-client/ipaclient/ipadiscovery.py b/ipa-client/ipaclient/ipadiscovery.py index 234e67e9b0f7102b1ee642fb3279793ff631ff9d..f91d4075aeafc71c3951ae094ddf075c143533f7 100644 --- a/ipa-client/ipaclient/ipadiscovery.py +++ b/ipa-client/ipaclient/ipadiscovery.py @@ -249,7 +249,7 @@ class IPADiscovery(object): if not ldapaccess and self.basedn is None: # Generate suffix from realm - self.basedn = str(realm_to_suffix(self.realm)) + self.basedn = realm_to_suffix(self.realm) self.basedn_source = 'Generated from Kerberos realm' root_logger.debug("Generated basedn from realm: %s" % self.basedn) diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py index d6e97b89ba3c6d288a4b9e60602a1918ae6e9e01..11433b4be832c1f6a79d17056e830c9582f3ca6e 100644 --- a/ipapython/ipautil.py +++ b/ipapython/ipautil.py @@ -831,7 +831,7 @@ def get_ipa_basedn(conn): % (info, IPA_BASEDN_INFO)) continue root_logger.debug("Naming context '%s' is a valid IPA context" % context) - return context + return DN(context) return None -- 1.7.11.4
_______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
