Hey, I reviewed a little (but important) part of the PR.
I'm planning to run the entire branch against an OpenStack provider, Digital Ocean and AWS EC2 soon, when I have the time. Best, Allard On Tue, Jan 10, 2017 at 9:46 AM anthony shaw <[email protected]> wrote: > I decided the best thing was to use this : > > https://github.com/apache/libcloud/pull/970 > > To verify the changes. It's going to be a complete driver, both > compute and storage using a real API. No Mocking, No funny-business. > Uses authentication, checks custom headers, file uploads and downloads > etc. We can use this in our release process. > > I've already found 3 bugs using it, including one nasty one. The > RawResponse class actually exposed the underlying httplib.HTTPResponse > class as a class property. I'm just writing a proxy class for it now. > > On Tue, Jan 10, 2017 at 7:41 PM, anthony shaw <[email protected]> > wrote: > > The regenerated provider tables? I had a merge conflict (well, many > > merge conflicts) on the way and re-running the table generator is the > > safest way to resolve those ones. > > > > They need running again, since the AWS regions are a dictionary, the > > ordering is random. > > > > On Tue, Jan 10, 2017 at 7:37 PM, Allard Hoeve <[email protected]> > wrote: > >> Hey Anthony, > >> > >> Wow, that is a lot of work :) Thanks for that. > >> > >> I started reviewing the branch, but it's 3k+, 3k- and I immediately ran > >> into the problem of "reviewing a patch that's too big". For example > >> < > https://github.com/apache/libcloud/pull/923/files#diff-f9b7105b8fba7a7f1a0ddec6ef14c8feR413 > >: > >> why is that added in? Seems unrelated, but you must've done that for a > >> reason. > >> > >> So before the review devolves into a bunch of nitpicks, how do you > suppose > >> we'd review most efficiently? > >> > >> Maybe you can go through the PR and comment yourself on why certain > things > >> are as they are? > >> > >> Best, > >> > >> Allard > >> > >> > >> On Mon, Jan 9, 2017 at 5:40 AM anthony shaw <[email protected]> > >> wrote: > >> > >>> This pull-request is FINALLY finished! It's taken me the best part of > >>> a year to complete it. > >>> > >>> https://github.com/apache/libcloud/pull/923 > >>> > >>> This last round of changes I had to completely rewrite the Storage API > >>> base classes. I've been testing using my GCP account and uploading and > >>> downloading files and it all looks good now. I've gone back and tested > >>> a bunch of random other drivers like GoDaddy and Dimension Data for > >>> which I have accounts for. > >>> > >>> I'm going to merge it and raise another PR this week with an > >>> integration suite module. This will have a driver and a FLASK web app > >>> with a tox definition to test the libcloud library from end to end. > >>> The way the httplib_ssl module is mocked out in our unit tests leaves > >>> a lot of room for mistakes. > >>> > >>> Things that visually don't make much sense but I don't have an account > to > >>> test > >>> - The code for Azure Blob leases looked fragile. This really needs > >>> testing properly > >>> - Multipart uploads should work for storage APIs, S3 has a custom API > >>> that is now disabled > >>> > http://docs.aws.amazon.com/AmazonS3/latest/dev/UsingRESTAPImpUpload.html > >>> - The Aliyun OSS driver had some really visible bugs in it, I doubt > >>> the existing driver works. We need integration testers for it. > >>> > >>> > >>> > >>> On Fri, Jan 6, 2017 at 8:05 PM, Samia, Michel <[email protected]> > wrote: > >>> > <html><bodyFor multipart upload you can use > >>> https://pypi.python.org/pypi/requests-toolbelt > >>> > > >>> > -----Original Message----- > >>> > From: anthony shaw [mailto:[email protected]] > >>> > Sent: Friday, January 6, 2017 6:07 AM > >>> > To: dev <[email protected]> > >>> > Subject: Re: [dev] [DISCUSS] Using requests instead of httplib > >>> > > >>> > Good news is I figured out a way of implementing the upload > >>> functionality using the requests package. > >>> > > >>> > > >>> > https://github.com/apache/libcloud/pull/923/commits/5e04dbce554830eca3f9812272076a2fbdbe7cdc > >>> > > >>> > I've tested it against the Google Cloud Storage account, downloaded > and > >>> uploaded a file using both the direct file_path option and the option > >>> passing a context manager (IOStream or ByteStream). > >>> > > >>> > The bad news is : > >>> > - The S3 multipart upload I've removed. I don't have time right now > to > >>> implement this feature from scratch > >>> > - The unit tests are all coupled to the private methods, the call > back > >>> system and a bunch of other bad coupling practices, so they are broken > BUT > >>> it does actually work > >>> > > >>> > It's nearly there. > >>> > > >>> > Ant > >>> > > >>> > On Fri, Jan 6, 2017 at 7:28 AM, anthony shaw < > [email protected]> > >>> wrote: > >>> >> it's more of an existential question :-) > >>> >> > >>> >> The _upload_object method inside the libcloud.storage.base submodule > >>> >> makes a 'raw' call to the LibcloudConnection, which will send the > top > >>> >> part of the HTTP request then some headers and leave the connection > >>> >> open (i.e. not read the response). > >>> >> > >>> >> Then, depending on the driver, the file and other things, it will > >>> >> callback one of the methods like _stream_data, which writes directly > >>> >> to the HTTP session using the `send()` method, which is only > available > >>> >> in httplib. > >>> >> > >>> >> httplib is a very low level library, requests is very high level. > You > >>> >> don't get access to the HTTP session directly in requests. > >>> >> > >>> >> That means that I would have to throw away the code we already have > >>> >> (which I am definitely in favour of in the long term since it is > >>> >> fragile) and replace it with requests' APIs for doing chunked > uploads > >>> >> using file streams. > >>> >> > >>> >> It would probably take me another day or two to finish that > >>> implementation. > >>> >> > >>> >> I always preached that you should change 1 thing at a thing, in > small > >>> >> amount, and keep testing. So far this has been more like pulling a > >>> >> thread on a sweater, I've touched every single file in the code base > >>> >> practically! > >>> >> The odds are, I will have missed something. So 2.0.0rc1 (if we do > call > >>> >> it that), despite my best intentions will introduce a new bug just > >>> >> based on the number of things I have changed. > >>> >> > >>> >> On Fri, Jan 6, 2017 at 1:11 AM, Tom Melendez <[email protected]> > wrote: > >>> >>> Hi Anthony, > >>> >>> > >>> >>> Nice job getting this going! > >>> >>> > >>> >>> Would you mind elaborating on this point? > >>> >>> "The raw connection still uses httplib. I decided it was too risky > to > >>> >>> swap that for requests' method of uploading files." > >>> >>> > >>> >>> Since you're going through the trouble, it would be ideal to go to > >>> >>> Requests completely. What's blocking us on the upload code > >>> >>> (Admittedly, I haven't studied the upload code)? Anything the > >>> community can do to help? > >>> >>> > >>> >>> Thanks, > >>> >>> > >>> >>> Tom > >>> >>> > >>> >>> > >>> >>> On Thu, Jan 5, 2017 at 3:43 AM, Chris DeRamus > >>> >>> <[email protected]> > >>> >>> wrote: > >>> >>> > >>> >>>> For what it's worth my company (DivvyCloud) has been using the > good > >>> >>>> work you've done now for almost six months. We had to make a few > >>> >>>> tweaks, but the core code contributed has worked flawlessly across > >>> >>>> AWS, Azure, OpenStack, Google, VMware and more. The only issue I > >>> >>>> believe that still stands which I've seen is an issue when > >>> >>>> LIBCLOUD_DEBUG is set to true. Logging doesn't appear to function > >>> >>>> properly, but that may have been addressed since your initial > >>> submission last year. > >>> >>>> > >>> >>>> Nice work on this and we sincerely appreciate the contribution. > >>> >>>> > >>> >>>> On Thu, Jan 5, 2017 at 12:21 AM, anthony shaw > >>> >>>> <[email protected]> > >>> >>>> wrote: > >>> >>>> > >>> >>>> > That package had a dumb error in it, I've since fixed it and > >>> >>>> > against a live API (GoDaddy). I've tested the following > scenarios > >>> >>>> > > >>> >>>> > - Applying a custom proxy via the environment variable > >>> >>>> > - Using libcloud.security to disable SSL verification > >>> >>>> > - Using libcloud.security to set a custom CA certificate > >>> >>>> > - Combining all of those scenarios > >>> >>>> > - Verification of custom headers applied by the driver using > >>> >>>> > Charles Proxy and inspecting the HTTP messages manually > >>> >>>> > - Decoding JSON messages - although this still uses the existing > >>> >>>> > methods, not the requests own json() decoder. > >>> >>>> > > >>> >>>> > IMO, this is ready to merge. I would like to test the raw > >>> >>>> > connections and file uploads if anyone has an account on one of > >>> those providers? > >>> >>>> > > >>> >>>> > On Thu, Jan 5, 2017 at 8:30 AM, Tomaz Muraus <[email protected]> > >>> wrote: > >>> >>>> > > Thanks for working on this again! > >>> >>>> > > > >>> >>>> > > Once we get a green light from people testing those changes, I > >>> >>>> > > propose > >>> >>>> to > >>> >>>> > > first roll out v2.0.0-rc1 and eventually after we are happy > with > >>> >>>> > > the stability call it v2.0.0. > >>> >>>> > > > >>> >>>> > > On Wed, Jan 4, 2017 at 6:20 AM, anthony shaw > >>> >>>> > > <[email protected] > >>> >>>> > > >>> >>>> > > wrote: > >>> >>>> > > > >>> >>>> > >> Hi, > >>> >>>> > >> > >>> >>>> > >> I tried doing this a year ago but put it in the 'too hard' > >>> bucket. > >>> >>>> > >> I've opened a PR (again) replacing the use of httplib with > the > >>> >>>> > >> requests package. > >>> >>>> > >> > >>> >>>> > >> The consequences are: > >>> >>>> > >> - Connection.conn_classes is no longer a tuple, it is > >>> >>>> > >> Connection.conn_class. There is no separation between a HTTP > >>> >>>> > >> and HTTPS connection. I could have just hacked around this > but > >>> >>>> > >> I updated all the tests instead > >>> >>>> > >> - Mock implementations no longer use the tuple as above > >>> >>>> > >> - We cannot support Python 3.2 officially anymore. Requests > >>> >>>> > >> does not support 3.2 > >>> >>>> > >> - The raw connection still uses httplib. I decided it was too > >>> >>>> > >> risky to swap that for requests' method of uploading files. > >>> >>>> > >> > >>> >>>> > >> > https://github.com/apache/libcloud/pull/923#partial-pull-mergin > >>> >>>> > >> g > >>> >>>> > >> > >>> >>>> > >> Please remote fetch this branch and test it out on some > working > >>> >>>> > >> code talking to real APIs. Mocks can only go so far. > >>> >>>> > >> > >>> >>>> > >> I've uploaded the package here - > >>> >>>> > >> http://people.apache.org/~anthonyshaw/libcloud/1.5.0.post0/ > >>> >>>> > >> > >>> >>>> > >> I would like to get this merged but would like some > additional > >>> >>>> > >> nods before it gets merged into trunk. > >>> >>>> > >> > >>> >>>> > >> Ant > >>> >>>> > >> > >>> >>>> > > >>> >>>> > >>> > > >>> > > >>> > NOTICE: This email and any attachments may contain confidential and > >>> proprietary information of NetSuite Inc. and is for the sole use of the > >>> intended recipient for the stated purpose. Any improper use or > distribution > >>> is prohibited. If you are not the intended recipient, please notify the > >>> sender; do not review, copy or distribute; and promptly delete or > destroy > >>> all transmitted information. Please note that all communications and > >>> information transmitted through this email system may be monitored by > >>> NetSuite or its agents and that all incoming email is automatically > scanned > >>> by a third party spam and filtering service > >>> > > >>> > </body></html> > >>> >
