Merge lp://qastaging/~djfroofy/txaws/newagent-767205 into lp://qastaging/txaws

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?

At the risk of being a total pain in the ass, here's what I would recommend:

1) split out the headers changes into a new branch, and attach it to bug #972432.
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.
3) do the work on the new agent in this branch and this ticket (bug #767205).

Thanks, will review tomorrow.

Sorry for the delay. Looks good to me.

Just a minor style nit: some of the code indents parameters to after the '(', and some only by 4 spaces. E.g.
{{{
def __init__(self, creds=None, endpoint=None, query_factory=None,
             parser=None, receiver_factory=None):
}}}
vs
{{{
def __init__(self, creds=None, endpoint=None, query_factory=None,
    receiver_factory=None):
}}}
The former is more readable.

Okay, if david-sarah reviewed it and said "Looks good to me", then the branch ought to be merged. Who has authority do that?

On Thu, Apr 12, 2012 at 5:02 PM, Zooko Wilcox-O'Hearn <email address hidden> wrote:
> Okay, if david-sarah reviewed it and said "Looks good to me", then the branch ought to be merged. Who has authority do that?

I'll take a look...

d

On Fri, Apr 13, 2012 at 12:43 AM, Duncan McGreggor <email address hidden> wrote:
> On Thu, Apr 12, 2012 at 5:02 PM, Zooko Wilcox-O'Hearn <email address hidden> wrote:
>> Okay, if david-sarah reviewed it and said "Looks good to me", then the branch ought to be merged. Who has authority do that?
>
> I'll take a look...

Okay, I just took a look. Looks like a review comment I left earlier
(needs fixing) hasn't been addressed by the author (unless I'm missing
something?).

Once that's either done or discussed, we can move this puppy forward :-)

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

> Sorry for the delay. Looks good to me.
>
> Just a minor style nit: some of the code indents parameters to after the '(',
> and some only by 4 spaces. E.g.
> {{{
> def __init__(self, creds=None, endpoint=None, query_factory=None,
> parser=None, receiver_factory=None):
> }}}
> vs
> {{{
> def __init__(self, creds=None, endpoint=None, query_factory=None,
> receiver_factory=None):
> }}}
> The former is more readable.

Yes, I agree and will address this.

Ok style fixes pushed to branch.

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!

On Sat, Apr 14, 2012 at 12:28 PM, Drew Smathers
<email address hidden> wrote:
>>
>> 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.

Do you mean bug #924459? (since bug #972432 is a duplicate of bug #640098 ...)

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

It looks like you mean bug #640098?

More soon...

> On Sat, Apr 14, 2012 at 12:28 PM, Drew Smathers
> <email address hidden> wrote:
> >>
> >> 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.
>
> Do you mean bug #924459? (since bug #972432 is a duplicate of bug #640098 ...)
>
> >> 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
>
> It looks like you mean bug #640098?

No, another typo, but I meant bug #924459. This has underlying support for the producer/receiver factory supports. This current ticket then just exposes that underlying support in s3 APIs.

> On Sat, Apr 14, 2012 at 12:28 PM, Drew Smathers
> <email address hidden> wrote:
> >>
> >> 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.
>
> Do you mean bug #924459? (since bug #972432 is a duplicate of bug #640098 ...)
>

Yes, this was a typo.

Merge away!

> Merge away!

Hi Duncan, am I the one who is supposed to do the merge, or does this need to be done by another developer?

On Fri, May 11, 2012 at 1:19 PM, Drew Smathers
<email address hidden> wrote:
>> Merge away!
>
> Hi Duncan, am I the one who is supposed to do the merge, or does this need to be done by another developer?

Yes! I've just added you to the team -- given your contributions (and
god-like patience!), I think that's more than reasonable :-)

You'll want to do the obvious (merge trunk into your branches, check
for conflicts, push, etc.), but you should be all set!

Let us know if you run into any problems.

d