Problem: plugin API missing pass-through cache interfaces #67
Conversation
dkliban
force-pushed
the
pass-through-cache
branch
2 times, most recently
from
8b19000
to
835aaed
Compare
Codecov Report
@@ Coverage Diff @@
## master #67 +/- ##
==========================================
+ Coverage 42.69% 42.95% +0.25%
==========================================
Files 22 22
Lines 712 724 +12
==========================================
+ Hits 304 311 +7
- Misses 408 413 +5
Continue to review full report at Codecov.
|
| Return an instance of the specific content by inspecting an artifact. | ||
|
|
||
| Plugin writers are expected to override this method with an implementation for a specific | ||
| content type. |
There was a problem hiding this comment.
I suggest to provide more detailed description which will serve as docs.
Maybe a basic example of implementation would be helpful.
I'd also add Return: section after Args: one. It helps to quickly see the whole signature of the method.
| raise ValueError(_("Relative path can't start with '/'. {0}").format(relative_path)) | ||
| return path.join(self.url, relative_path) | ||
|
|
||
| def get_remote_artifact_content_type(self, relative_path=None): |
There was a problem hiding this comment.
I was looking at how this and Content API changes are used in pulp/pulpcore#1 and currently it's in one place and no processing/logic added between two calls:
c_type = remote.get_remote_artifact_content_type(rel_path)
content = c_type.init_from_artifact_and_relative_path(artifact, rel_path)
So I wonder if it makes sense for the plugin writer to create only one method (here on a Remote, and not force/specify how to extend the Content model at all). It is possible that plugin writers already have methods for creating their content from artifact (e.g. upload case) and we are forcing them to have another similar method. I'm leaning towards giving plugins more freedom to organize their code.
Maybe your way is simpler for plugin writer, I'm open for discussion.
Also if you foresee a separate usage of Content.init_from_artifact_and_relative_path and Remote.get_remote_artifact_content_type, then it's a compelling argument to keep them separate.
There was a problem hiding this comment.
I think the one shot upload could use the Content.init_from_artifact_and_relative_path. That's why I did not want to combine the two.
There was a problem hiding this comment.
Ok, fine with me, just tried to make it simpler.
dkliban
force-pushed
the
pass-through-cache
branch
from
835aaed
to
d7659f1
Compare
Solution: add interfaces that enable pull-through cache for plugins This patch extends the Remote API. It adds an interface for determining the full URL of a RemoteArtifact by examining the relative path and the Remote's url. It also adds an interface for determining the type of content that should be available at a relative path of a remote. These interfaces are used by the content app when serving content in the pull-through cache mode. The content app needs to determine what URL is actually being requested and what kind of content is being requested by the client. This patch also extends the Content API. It adds a new constructor method to the Content model. This new interface is a constructor that take an Artifact and a relative path to produce an unsaved instance of a specific content type. Plugin writers are expected to provide specific implementations of these methods. re: #3894 https://pulp.plan.io/issues/3894
dkliban
force-pushed
the
pass-through-cache
branch
from
d7659f1
to
2226678
Compare
Solution: add interfaces that enable pass-through cache for plugins
This patch extends the Remote API. It adds an interface for determining the full URL of a RemoteArtifact
by examining the relative path and the Remote's url. It also adds an interface for determining the type of
content that should be available at a relative path of a remote. These interfaces are used by the content
app when serving content in the pass-through cache mode. The content app needs to determine what URL is
actually being requested and what kind of content is being requested by the client.
This patch also extends the Content API. It adds a new constructor method to the Content model. This new
interface is a constructor that take an Artifact and a relative path to produce an unsaved instance of a
specific content type.
Plugin writers are expected to provide specific implementations of these methods.
re: #3894
https://pulp.plan.io/issues/3894