Code review comment for lp://qastaging/~djfroofy/txaws/newagent-767205

On Sat, Apr 14, 2012 at 12:28 PM, Drew Smathers
<email address hidden> wrote:
>> This change looks good to me at first glance (reading the diff on the merge
>> proposal; I haven't branched the code yet). In particular, the amz_headers
>> functionality is much-needed.
>>
>> Drew, I don't see anything about the new web client usage, only headers and
>> body producer. Is that coming in a future change?
>>
>
> Ok, so I had to remember myself while staring at the diff. The code for agent is in the prerequisite branch: lp:~djfroofy/txaws/modernize-924459
>
>
>> At the risk of being a total pain in the ass, here's what I would recommend:
>>
>
> No, no worries.
>
>> 1) split out the headers changes into a new branch, and attach it to bug
>> #972432.
>
> #972432 must be a duplicate of #640098; the amz_headers argument has been merged already.  To summarize what this diff is adding: Support for plugging in Body Producers and Receivers in s3.client (this is made possible by: lp:~djfroofy/txaws/modernize-924459).
>
> There are actually no changes related to headers in this patch. Rather, I'm adding receiver_factory as __init__() argument to S3Client and body_producer as argument to regular S3 api calls.
>
> Cutting to the point, I think #972432 needs to be reviewed first.
>
>> 2) create a new bug for the body producer/receiver factory feature, split that
>> code out, and attach it to there in its own branch.
>
> That was in the scope of #972432
>
>> 3) do the work on the new agent in this branch and this ticket (bug #767205).
>
> Again: #972432 is where that functionality is.
> --
> https://code.launchpad.net/~djfroofy/txaws/newagent-767205/+merge/93312
> You are reviewing the proposed merge of lp:~djfroofy/txaws/newagent-767205 into lp:txaws.

Thanks for catching me up, Drew -- this is good stuff.

I'm going to be in the air tomorrow, on the way to the OpenStack
design summit and conference, but will branch all the code and check
out the associated tickets, to make sure I haven't missed anything.

Regardless, it looks like we've got a pending merge on our hands. This
code should land in trunk within a day or two...

Thanks again!