Add support for adding pull-through content to associated repositories#6224
Add support for adding pull-through content to associated repositories#6224
Conversation
| log.info(f"RemoteArtifact for {url} already exists.") | ||
| if ca.relative_path == content_artifact.relative_path: | ||
| # Side effect used by pull-through-caching in _stream_remote_artifact | ||
| remote_artifact.content_artifact = ca |
There was a problem hiding this comment.
Apart from the question, why we should rename the variable in the first place, does remote_artifact exist here?
Also should we save "content_artifact" on the model when we set it? Maybe the comment is missing some nasty details to understand the codeflow here.
There was a problem hiding this comment.
remote_artifact exists in the method definition.
I don't understand your second question, but I am wondering if I even need to use a side-effect to get the newly saved content_artifact for the pull-through method. This is a private method and searching through the code-base no one uses this method besides the content app, so maybe I can just change the return type to a dictionary of all the newly created objects.
There was a problem hiding this comment.
So content_artifact isn't even a field on the remote_artifact?
But yes the returning the proper stuff sounds better.
So sad that with python it's usually hard to guess what is a private and what a public interface...
gerrod3
force-pushed
the
pull-through-repo-add
branch
from
57606c3 to
3118fc1
Compare
| @middleware | ||
| async def guid(request, handler): | ||
| """Sets the django_guid for each request.""" | ||
| set_guid(generate_guid()) | ||
| return await handler(request) | ||
|
|
||
|
|
There was a problem hiding this comment.
This sounds like a good change in general. Maybe that's a different story, but should we look for the cid header in the request to allow following correlations even through content access?
There was a problem hiding this comment.
Maybe yeah, we would also need to expose the value to be used in logging for the content app, currently this change is solely to allow for dispatching tasks from the content app.
gerrod3
force-pushed
the
pull-through-repo-add
branch
from
3118fc1 to
1c41bb0
Compare
gerrod3
force-pushed
the
pull-through-repo-add
branch
3 times, most recently
from
d0a7252 to
7f995ef
Compare
gerrod3
force-pushed
the
pull-through-repo-add
branch
from
7f995ef to
8ad5f0f
Compare
| finally: | ||
| await content.adelete() | ||
| await repo.adelete() | ||
| await remote.adelete() | ||
| await distro.adelete() |
There was a problem hiding this comment.
I don't think we need that... Django unittests usually run in a transaction that never gets committed.
2 participants
No description provided.