Add mirrors #2795
Add mirrors #2795
Conversation
|
Let's rebase and get this in! |
|
|
||
| namespace mamba | ||
| { | ||
| class CURLHandle; |
There was a problem hiding this comment.
This should not be exposed in public headers.
| virtual std::string format_url(const std::string& path) const; | ||
| virtual std::string get_auth_header(const std::string& path) const; | ||
| virtual bool needs_preparation(const std::string& path) const; | ||
| virtual void prepare(const std::string& path, CURLHandle& handle); |
There was a problem hiding this comment.
Private virtual methods should be preferred.
|
|
||
| protected: | ||
|
|
||
| Protocol m_protocol; |
There was a problem hiding this comment.
Data should not me exposed to inheriting classes, prefer private data and protected accessors.
| HTTPMirror(const std::string& url); | ||
| ~HTTPMirror(); | ||
|
|
||
| bool authenticate(CURLHandle& handle, const std::string& user, const std::string& password); |
There was a problem hiding this comment.
Design: this requires the user to know the exact type or mirror, which violates the LSP principle.
| std::string get_repo(const std::string& repo) const; | ||
| std::string get_auth_url(const std::string& repo, const std::string& scope) const; | ||
| std::string get_manifest_url(const std::string& repo, const std::string& reference) const; | ||
| std::string get_preupload_url(const std::string& repo) const; |
There was a problem hiding this comment.
These should be private methods.
| @@ -446,6 +446,21 @@ namespace mamba | |||
| return *this; | |||
| } | |||
|
|
|||
| CURLHandle& CURLHandle::set_url(const std::string& url, const proxy_map_type& proxies) | |||
There was a problem hiding this comment.
This should not be required, ot deserves a better name if we need to reset the URL of an existing handler.
|
|
||
| // Utility function | ||
| // TODO leave it here or put it in a namespace, or move it to utils? | ||
| std::pair<std::string, std::string> split_path_tag(const std::string& path); |
There was a problem hiding this comment.
Should retrurn a struct (this is more expressive)
| std::size_t max_ranges = 256; | ||
| }; | ||
|
|
||
| class Mirror |
There was a problem hiding this comment.
Documentation about what are mirrors and general expectation about what they provide would help here.
Also some member functions meaning coudl be clearified below.
|
|
||
| const std::string m_url; | ||
|
|
||
| // TODO put these in a struct? |
| { | ||
| public: | ||
|
|
||
| HTTPMirror(const std::string& url); |
There was a problem hiding this comment.
explicit for single argument constructors.
| { | ||
| public: | ||
|
|
||
| Mirror(const std::string& url); |
There was a problem hiding this comment.
explicit for single argument constructors.
| }; | ||
|
|
||
| // Utility function | ||
| // TODO leave it here or put it in a namespace, or move it to utils? |
There was a problem hiding this comment.
I guess it could be moved in string utilities?
|
|
||
| void OCIMirror::prepare(const std::string& path, CURLHandle& handle) | ||
| { | ||
| auto [split_path, split_tag] = split_path_tag(path); |
|
|
||
| if (data && data->token.empty() && need_auth()) | ||
| { | ||
| std::string auth_url = get_auth_url(split_path, m_scope); |
| { | ||
| std::string auth_url = get_auth_url(split_path, m_scope); | ||
|
|
||
| // TODO to be checked |
There was a problem hiding this comment.
Are the todos in this function still relevant?
|
|
||
| OCIMirror::AuthData* OCIMirror::get_data(const std::string& path) const | ||
| { | ||
| auto [split_path, _] = split_path_tag(path); |
|
|
||
| namespace mamba | ||
| { | ||
| TEST_SUITE("mirror") |
There was a problem hiding this comment.
I know it's planned but I note this as a future reminder: needs test, and one of the first one would be to implement a test mirror and make it work with the mechanism handling mirrors (no downloading required). Then for tests requiring some downloading, I suppose they will need to be somewhat optional?
|
Let's address the comments when we plug the mirrors in the downloaders |
No description provided.