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

Revision history for this message
Drew Smathers (djfroofy) 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.

« Back to merge proposal