feat: Add blocking client #189
Conversation
Uses the reqwuest/blocking feature to add a client that can be used from sync rust. This is gated behind the `blocking` feature. Removed test-registry from default-features. Signed-off-by: Dave Tucker <[email protected]>
dave-tucker
requested review from
thomastaylor312,
bacongobbler and
flavio
as code owners
There was a problem hiding this comment.
I am all onboard with having a blocking client as well! The main focus of the comments here are trying to reuse as much code as possible between the two clients. Some parts will just be easier or need to be copy/pasted and modified, but I think a good chunk of them could be shared, which will reduce maintenance burden/problems
| @@ -8,8 +8,6 @@ on: | |||
| jobs: | |||
| build: | |||
| runs-on: ubuntu-latest | |||
| strategy: | |||
There was a problem hiding this comment.
Any reason why this was removed? We had this on here so other tests could still run on the OS it didn't fail on
There was a problem hiding this comment.
Yes. strategy only applies to matrix jobs. From the docs
jobs.<job_id>.strategy.fail-fast applies to the entire matrix.
| const MIME_TYPES_DISTRIBUTION_MANIFEST: &[&str] = &[ | ||
| IMAGE_MANIFEST_MEDIA_TYPE, | ||
| IMAGE_MANIFEST_LIST_MEDIA_TYPE, | ||
| OCI_IMAGE_MEDIA_TYPE, | ||
| OCI_IMAGE_INDEX_MEDIA_TYPE, | ||
| ]; | ||
|
|
||
| const PUSH_CHUNK_MAX_SIZE: usize = 4096 * 1024; | ||
|
|
||
| /// Default value for `ClientConfig::max_concurrent_upload` | ||
| pub const DEFAULT_MAX_CONCURRENT_UPLOAD: usize = 16; | ||
|
|
||
| /// Default value for `ClientConfig::max_concurrent_download` | ||
| pub const DEFAULT_MAX_CONCURRENT_DOWNLOAD: usize = 16; | ||
|
|
||
| /// Default value for `ClientConfig:default_token_expiration_secs` | ||
| pub const DEFAULT_TOKEN_EXPIRATION_SECS: usize = 60; | ||
|
|
||
| static DEFAULT_USER_AGENT: &str = concat!(env!("CARGO_PKG_NAME"), "/", env!("CARGO_PKG_VERSION")); |
There was a problem hiding this comment.
Let's move this out of here and in client.rs to lib.rs so it isn't duplicated across both blocking and the normal client
| } | ||
|
|
||
| #[cfg(feature = "blocking")] | ||
| impl SyncTokenCache { |
There was a problem hiding this comment.
Can you take the logic in these functions that is the same and break them out to functions that can be reused rather than copy pasting?
| /// The OCI spec technically does not allow any codes but 200, 500, 401, and 404. | ||
| /// Obviously, HTTP servers are going to send other codes. This tries to catch the | ||
| /// obvious ones (200, 4XX, 5XX). Anything else is just treated as an error. | ||
| fn validate_registry_response(status: reqwest::StatusCode, body: &[u8], url: &str) -> Result<()> { |
There was a problem hiding this comment.
This function looks like it can be shared between both clients
| /// | ||
| /// Location may be absolute (containing the protocol and/or hostname), or relative (containing just the URL path) | ||
| /// Returns a properly formatted absolute URL | ||
| fn location_header_to_url( |
There was a problem hiding this comment.
This method could probably be moved to the ClientConfig struct as a pub(crate) so it can be reused across the clients
| } | ||
|
|
||
| /// Convert a Reference to a v2 manifest URL. | ||
| fn to_v2_manifest_url(&self, reference: &Reference) -> String { |
There was a problem hiding this comment.
Same note here around moving this to a method on ClientConfig for better sharability. Basically all the rest of the methods until line 1302 can probably be moved over there for better reuse
| } | ||
|
|
||
| /// A client configuration | ||
| pub struct ClientConfig { |
There was a problem hiding this comment.
We should do whatever we need to here to make sure this is shared across both blocking and non-blocking clients. Pretty much everything here until the tests can be shared across clients
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod test { |
There was a problem hiding this comment.
I would really prefer if we could find a way to test both clients using the same tests. Even if we just had an enum with the blocking and non blocking client (that we only used for tests) that matches on the client and calls the right thing
|
@thomastaylor312 thanks for the review. I'll address your comments and try and get another revision done with more re-use and less copy/paste some time in the next couple of weeks 😄 |
Uses the reqwuest/blocking feature to add a client that can be used from sync rust. This is gated behind the
blockingfeature.Removed test-registry from default-features.