Paul, Thanks for reviewing my code. Replies to specific comments below:
On Mon, Apr 26, 2010 at 4:10 PM, Paul Querna <[email protected]> wrote: > On Mon, Apr 26, 2010 at 10:58 AM, <[email protected]> wrote: > > Author: oremj > > Date: Mon Apr 26 17:58:12 2010 > > New Revision: 938156 > > > > URL: http://svn.apache.org/viewvc?rev=938156&view=rev > > Log: > > Add Enomaly ECP Driver. > > > > Author: Viktor Stanchev <[email protected]> > > Signed-off-by: Jeremy Orem <[email protected]> > ...snip... > > + def request(self, *args, **kwargs): > > + return super(ECPConnection, self).request(*args, **kwargs) > > Any reason this needs to be in there? > > It seems like it doesn't do anything. I guess I was copying and pasting and then deleting things that were unnecessary and that was left over. I'll remove it. > .... > > + def _encode_multipart_formdata(self, fields): > > + BOUNDARY = '----------ThIs_Is_tHe_bouNdaRY_$' > > + CRLF = '\r\n' > > + L = [] > > + for i in fields.keys(): > > + L.append('--' + BOUNDARY) > > + L.append('Content-Disposition: form-data; name="%s"' % i) > > + L.append('') > > + L.append(fields[i]) > > + L.append('--' + BOUNDARY + '--') > > + L.append('') > > + body = CRLF.join(L) > > + content_type = 'multipart/form-data; boundary=%s' % BOUNDARY > > + header = {'Content-Type':content_type} > > + return header, body > > I thought that httplib would support doing this, but it appears it > doesn't support the multipart form data encoding: > http://bugs.python.org/issue3244 > > I personally kinda wish the boundary was randomly generated, and > checked to make sure its content was not inside the content. > > I guess i would at least be more happy with something like this: > boundary = os.urandom(16).encode('hex') > > (Related, BOUNDARY isn't really a good style, it should be lower case, > and I am concerned that this code was copied from > <http://code.activestate.com/recipes/146306/> without any attribution) > > You seem to be correct that the code is from there. Sorry about that. I didn't copy it intentionally. It was given to me without any context by someone when I was trying to figure out why the request wasn't working without it. I don't mind adding a link/reference in the code to indicate its source. I'll look into making the boundary random, but the benefit is very little. > .... snip .... > > +class ECPNodeDriver(NodeDriver): > > + def __init__(self, user_name, password): > > + """ > > + Sets the username and password on creation. Also creates the > connection > > + object > > + """ > > + self.user_name = user_name > > + self.password = password > > Shouldn't these just use the built in key / secret variables, and let > the default NodeDriver __init__ do this? > > That would make sense. I'm not sure why I did this. Maybe because ECPConnection doesn't have a method called "connect", but I guess I can just add to it "def connect(self): pass" > ... snip ... > > + def _to_node(self, vm): > > + """ > > + Turns a (json) dictionary into a Node object. > > + This returns only running VMs. > > + """ > > + > > + #Check state > > + if not vm['state'] == "running": > > + return None > > + > > + #IPs > > + iplist = [interface['ip'] for interface in vm['interfaces'] if > interface['ip'] != '127.0.0.1'] > > + > > + #Create the node object > > + n = Node( > > + id=vm['uuid'], > > + name=vm['name'], > > + state=NodeState.RUNNING, > > + public_ip=iplist, > > + private_ip=iplist, > > + driver=self, > > + ) > > This seems less than ideal, setting public_ip and private_ip to the > same lists. It likely needs code similiar to the slicehost driver, > which splits the two based on the published private subnets. We can > likely move that function from the slicehost driver to a util area in > the base library. > > We don't distinguish between public and private IPs either, so that would be nice. Meanwhile, can I copy the function? > It also does not throw InvalidCredsException anywhere, which is bad > because then we can't tell if there is a error in the username or > password -- so parse_error should be extended to detect those errors > and throw the right exception. > > I'll add the error handling. > One last thing, uuid is imported, but unused, so pyflakes complains. > > I'll remove it. I didn't notice that. > Thanks for getting the driver in, > > Paul > No problem. I'll work on fixing everything mentioned. I'll assume it's ok to copy the code from slicehost unless you tell me otherwise. By the way, I would appreciate it if you could provide some testing code that I can run to make sure the driver works on a live server. I would imagine I'm not the first person to need to do that. - Viktor
