camallo / dkregistry-rs

A pure-Rust asynchronous library for Docker Registry API v2
Apache License 2.0
62 stars 39 forks source link

v2/manifest: add get_manifest_and_ref method #85

Closed steveej closed 5 years ago

steveej commented 5 years ago

This method returns the manifestref in addition to the manifest. As part of the migration to reqwest the method is a rewrite of get_manifest(..), which now wraps the new one and omits the manifestref from the result.

lucab commented 5 years ago

@steveeJ a couple of questions:

steveej commented 5 years ago
  • do we want to introduce a structure holding both manifest-blob and ref?

I'd say yes, because they are both retrieved in the same API call. I don't see anything wrong with getting both at once.

  • do we want to directly replace current get_manifest with this?

They're almost equivalent, except that one returns the manifestref too, which is simply omitted. I trust Rust to be sufficiently efficient here. Since the existing tests aren't failing it at least is shown to work :-)

steveej commented 5 years ago

I want to explain the context a bit. In cincinnati we need the manifestref in order to get the labels from the separate quay API. Passing the manifestref on like this PR implements it is the most efficient way obvious to me. See the relevant code in a snapshot of my working branch.

lucab commented 5 years ago

@steveeJ I think both points above makes sense to implement. Do you prefer to land this as-is or to make the changes now?

steveej commented 5 years ago

I think both points above makes sense to implement. Do you prefer to land this as-is or to make the changes now?

I'd say it can be merged as is now. To clarify I interpreted your questions differently earlier. Let me go again:

do we want to introduce a structure holding both manifest-blob and ref?

I think that's not necessary since we have the type alias which is convenient enough.

do we want to directly replace current get_manifest with this?

Not necessarily since it would break compatibility with the existing tests and other code there might be.