Push and Pull send progress steps to supplied block; pushing raises an error if unauthorized.

[mrflip]

This patch lets me show feedback during the long slow process of pushing or pulling an image.

• A block supplied to Image#push or Image.create will be called with the parsed hash of each json message streamed back from the server.
• create and push now raise errors if an error response is returned from the server
• The VCR example for creating an image was actually that of a 401 Unauthorized request. I faked data from a successful pull into the 'pushes_the_Image.yml' VCR record, and used the old version as the test case for an unauthorized call.

I'm not quite sure how I should be handling exceptions in all this.

First, I'm assuming it's fair to raise an exception if any response from the server shows an error -- but if there are known-harmless errors out there, they will now be explodey.

Also, if an error condition occurs in the response_block, it is caught and re-thrown as an Excon error. So I'm throwing a generic error inside the block, then catching all excon errors and re-throwing only the ones I recognize (Unauthorized for push, Not Found for pull) as Docker errors. Let me know if you prefer some other strategy.

This obviates the PR in #209, as the last ID is retrieved naturally in the response block

[mrflip]

So two code nanny alerts are tripping about a complex method, which I assume is because I'm assembling the response block in the method itself:

  def push(creds = nil, options = {}, &caller_blk)
    # (...setup stuff...)
    resp_blk = lambda do |chunk, *_|
      steps = Docker::Util.fix_json(chunk)
      steps.each do |step|
        raise step['error'] if step['error']
        caller_blk.call(step) if caller_blk
      end
    end
    #
    connection.post("/images/#{repo}/push", opts, :headers => headers, :response_block => resp_blk)
    self
  end

So I could fix that by

  def push(creds = nil, options = {}, &caller_blk)
    # (...setup stuff...)
     resp_blk = get_response_block(caller_blk)
    #
    connection.post("/images/#{repo}/push", opts, :headers => headers, :response_block => resp_blk)
    self
  end

  def get_response_block(caller_blk)
    lambda do |chunk, *_|
      steps = Docker::Util.fix_json(chunk)
      steps.each do |step|
        raise step['error'] if step['error']
        caller_blk.call(step) if caller_blk
      end
    end
  end

But at that point I have a method which takes a block calling a method to make a block; that block, given content by the later call of the first method, will call back to the callback block it was sent on behalf of the caller of the first method.

Which, I argue, is much less clear.
But I'll switch to the second version if you like.

[mrflip]

I pushed another commit that passes the cane nanny; I'd argue it's worse but you may find it more readable.

The problem is that the block for create needs to smuggle back the id of the final layer seen, and so .create has to hand off a little bit of scope to do so.. The second version uses an array, which is passed by reference, as a pretense to accomplish this dishonestly. If y'all have suggestions about how to do this with a scalar instead, I would be less offended by the split-out block version.

[nahiluhmot]

Supporting progress steps is an interesting feature, however your README.md update is unrelated and should go in a separate pull. It's much easier to review and reason about pulls when they're single-purpose, so please remove that for now. Also, you're going to have to rebase off of master now that #220 is merged.

[nahiluhmot]

rescue StandardError => err is redundant, prefer rescue => err

[mrflip]

The push method tripped an ABC complexity flag, and this seemed like a reasonable idea anyway. Pulling the case statement out of the rescue block without screwing up backtraces seemed harder.

[mrflip]

OK, rebased off master and code review points addressed. Please give this the same close going-over.

---

Since this change adds error checking to the .create and #push methods, I had to first fix a problem where the existing tests were recordings of "unauthorized error" responses

The tests in push took the image tianon/true and did a build with no changes -- giving an image named both tianon/true and <USERNAME>/true. Since tianon/true came first in the repo tags, the push specs tried to push to tianon/true, and so error.

My solution (in the first commit) is to make the created image non-identical to tianon/true, by adding a gratuitous environment variable to the build script. That leaves the image at hand named only <USERNAME>/true, and so it sails off to your actual account as it should.

Similarly, the test in spec/vcr/Docker_Image/_push/when_there_are_no_credentials/still_pushes.yml was not actually pushing to the local registry. I could not get the local registry to work at all (http vs https) and so what's there is a dummied-up response, but at least it's to the correct request.

Two other minor things I'll drop into line comments.

[mrflip]

If I don't supply a :tag part (meaning "all tags for that root", not ":latest"), the optional regexp capture becomes a prefix match. If I do supply a :tag part, the optional capture group is irrelevant, because none of the repo tags have two post-slash :s.

I would prefer something less magical, but anything straightforward I could come up with got complicated fast for dealing with things like localhost:5000/alice:chapeau and localhost:5000.