Move single-artifact related code into core #3476
Conversation
ref #3607 https://pulp.plan.io/issues/3607 Required PR: pulp/pulp#3476
ref #3607 https://pulp.plan.io/issues/3607 Required PR: pulp/pulp#3476
| @@ -138,6 +138,27 @@ def natural_key(self): | |||
| return tuple(getattr(self, f) for f in self.natural_key_fields()) | |||
|
|
|||
|
|
|||
| class SingleArtifactContent: | |||
There was a problem hiding this comment.
Why is this a mixin instead of a subclass of Content? Multiple inheritance (mixins) are generally considered to be bad practice.
Why not add these convenience properties to Content?
There was a problem hiding this comment.
Will make SingleArtifactContent a subclass 👍
The reason I didn't want to add these to Content is that I didn't want plugin writers to accidentally call them if they're dealing with Content that may have multiple artifacts.
There was a problem hiding this comment.
I'm cool with adding them to Content though and maybe being explicit in the docstring that they're not for use with Content that can have multiple artifacts?
There was a problem hiding this comment.
Actually, after looking at this harder .. Can you help me understand the value of this? Methods with 1 line of code raises a yellow flag. How is this better than the caller just doing the same line of code?
fixes #3607 fixes https://pulp.plan.io/issues/3607 Required PR: pulp/pulp_file#83
ref #3607 https://pulp.plan.io/issues/3607 Required PR: pulp/pulp#3476
ref #3607 https://pulp.plan.io/issues/3607 Required PR: pulp/pulp#3476
|
|
||
| headers = self.get_success_headers(serializer.data) | ||
| return Response(serializer.data, status=status.HTTP_201_CREATED, headers=headers) | ||
| return super().create(request) |
There was a problem hiding this comment.
Does this need to be overridden at all if we're just calling super()?
|
Abandoning this work. May create a serializer in the future to handle this. |
Looks like django requires certain features of hostnames like a "." so using `get_host()` was not a good idea. Using a known valid hostname instead. fixes pulp#3476 https://pulp.plan.io/issues/3476
fixes #3607
fixes https://pulp.plan.io/issues/3607