incubator-libcloud mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Paul Querna <p...@querna.org>
Subject [libcloud] Re: svn commit: r938156 - in /incubator/libcloud/trunk: libcloud/ libcloud/drivers/ test/ test/fixtures/ecp/
Date Mon, 26 Apr 2010 20:10:18 GMT
On Mon, Apr 26, 2010 at 10:58 AM,  <oremj@apache.org> 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 <viktor@enomaly.com>
> Signed-off-by: Jeremy Orem <jeremy.orem@gmail.com>
...snip...
> +    def request(self, *args, **kwargs):
> +        return super(ECPConnection, self).request(*args, **kwargs)

Any reason this needs to be in there?

....
> +    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)

.... 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?

... 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.

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.

One last thing, uuid is imported, but unused, so pyflakes complains.

Thanks for getting the driver in,

Paul

Mime
View raw message