r98380 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r98379‎ | r98380 | r98381 >
Date:22:48, 28 September 2011
Author:ben
Status:resolved (Comments)
Tags:
Comment:
added some error handling around empty or malformed strings, allowed whitespace in the input variables. follow up to revision r98364
Modified paths:
  • /trunk/debs/wikimedia-task-dns-auth/gen-zones (modified) (history)

Diff [purge]

Index: trunk/debs/wikimedia-task-dns-auth/gen-zones
——@@ -31,12 +31,19 @@
3232 # $langlist_wikimedia:wikimedia-lb.wikimedia.org.
3333 domainlist = {}
3434 domainmap = open(domainmaplist)
 35+ lineno=0
3536 for map in domainmap.readlines():
36 - if map[0] == "#":
 37+ lineno = lineno + 1
 38+ map = map.strip()
 39+ # skip blank lines and comments
 40+ if not map or map.startswith("#"):
3741 continue
38 - map = map.strip()
39 - var, value = map.split(':')
40 - domainlist[var] = value
 42+ try:
 43+ var, value = map.split(':')
 44+ except ValueError, e:
 45+ print "Syntax error in %s at line %s: %s. Quitting." % (domainmaplist, lineno, e)
 46+ exit(1)
 47+ domainlist[var.split()] = value.split()
4148
4249 # All substs[var] in this loop contain a list of CNAMES to the geodns record for
4350 # all language subdomains

Follow-up revisions

RevisionCommit summaryAuthorDate
r98415bug fixing - used split when I meant strip, exit needs to come from sys to wo...ben16:25, 29 September 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r98364Follow up to r98360.laner20:27, 28 September 2011

Comments

#Comment by Mark Bergsma (talk | contribs)   11:30, 29 September 2011

Good changes for robustness, but I don't get domainlist[var.split()] = value.split(). I don't think those variables need to be split still, and in any case, split() returns a list, which can't be used a key for a dict. Do you mean strip() perhaps?

We should definitely test this code before we push DNS changes with it

#Comment by Bhartshorne (talk | contribs)   16:26, 29 September 2011

Yes, I did mean strip. Thank you.

#Comment by Mark Bergsma (talk | contribs)   11:50, 29 September 2011

exit(1) should probably be sys.exit(1), I don't believe exit() is a global built-in function.

We should also check how the calling script(s) react to that - they should under no condition push out a partially written out zone file.

#Comment by Bhartshorne (talk | contribs)   16:27, 29 September 2011

ah yes. I keep getting fooled by the python interpreter, in which it does work (http://docs.python.org/library/constants.html#exit).

#Comment by Ryan lane (talk | contribs)   19:01, 29 September 2011

gen-zones is the last script in the chain. If this failed, it would exit out before the zones were created. Everything else would run, but all the other changes wouldn't affect the currently running DNS.

Status & tagging log