-
Notifications
You must be signed in to change notification settings - Fork 123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Core 3.70 compatibility #3876
Core 3.70 compatibility #3876
Conversation
94104a1
to
f0bf4ba
Compare
@@ -88,7 +88,7 @@ class ModulemdDefaultsSerializer(NoArtifactContentSerializer): | |||
|
|||
module = serializers.CharField(help_text=_("Modulemd name.")) | |||
stream = serializers.CharField(help_text=_("Modulemd default stream.")) | |||
profiles = CustomJSONField(help_text=_("Default profiles for modulemd streams.")) | |||
profiles = serializers.JSONField(help_text=_("Default profiles for modulemd streams.")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain the distinction between serializers.JSONField
, JSONDictField
, and JSONListField
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
serializers.JSONField
(drf) accept any json type (bool, str, object, array, ...) and has openapi typeany
.JSONDictField
(custom) validates it's a json object (aka dict) and has openapi typeobject
.JSONListField
(custom) validates it's a json array (aka list) and has openapi typearray
.
2 and 3 subclasses from 1 and then adds the layer of validation.
But actually I found that my implementation of 2 and 3 is wrong.
I should either:
a) add the correct version of these fields here to get the types and validation right
b) use the serializers.JSONField which will change the types we use in openapi to any
(which will probably not break ruby bindings this time because the new bindindg generator image probably supports any type)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
B) is what the python plugin just did I believe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I'll do that here as there is already a lot going on
pulp_rpm/tests/conftest.py
Outdated
@pytest.fixture | ||
def pulp_requests(bindings_cfg): | ||
"""Uses requests lib to issue an http request to pulp server using pulp_href. | ||
|
||
Example: | ||
>>> response = pulp_requests("get", "/pulp/api/v3/.../?repository_version=...") | ||
>>> type(response) | ||
requests.Response | ||
""" | ||
ALLOWED_METHODS = ("get", "update", "delete", "post") | ||
auth = (bindings_cfg.username, bindings_cfg.password) | ||
host = bindings_cfg.host | ||
|
||
def _pulp_requests(method: str, pulp_href: str, body=None): | ||
if method not in ALLOWED_METHODS: | ||
raise ValueError(f"Method should be in: {ALLOWED_METHODS}") | ||
url = urljoin(host, pulp_href) | ||
request_fn = getattr(requests, method) | ||
return request_fn(url, auth=auth) | ||
|
||
return _pulp_requests | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I have incidentally created a similar thing here:
https://github.com/pulp/pulp_ansible/blob/84a32718cbe18216b4067a50eea222d713c818ed/pulp_ansible/tests/functional/api/collection/v3/test_collection.py#L29
In any case, you can use requests.request(method, *)
instead of the getattr dance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. I guess I'll addopt the session approach.
c3de487
to
6621b87
Compare
Deadlock on import-export: |
required=False, | ||
help_text=_("A JSON document describing config.repo file"), | ||
) | ||
|
||
def to_representation(self, instance): | ||
data = super().to_representation(instance) | ||
# Import workflow may cause these fields to be stored as "" in the database |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a separate codepath that ought to be adjusted here? Anything to fix with a data migration? Ideally we want total consistency w/ how values are stored in a field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that we can fix import-export workflow to ensure it stores null values and add migration to fix empty strings. I can open an issue for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of this looks reasonable and most of it looks like a cleanup over what came before. Nice job!
Please squash before merging
6621b87
to
1e9e9ec
Compare
This does: * Bumps lower and upper bounds for pulpcore: >=3.70, <3.85 * Removes pulp_smash and replaces it with similar fixtures * Adapt to the new bindings changes caused by openapi-generator bump * Revert the usage of CustomJSONField (to serializer.JSONField) due to bindings changes.
1e9e9ec
to
08a5100
Compare
#3865 declares compatibility with pulpcore 3.70, but the PR is getting a lot of errors not related to the changes.
For example, I can see a lot of binding validation errors which were somehow expected because of our bump of binding generator image version (see here).
I'm creating this PR so other errors related to the compatibility can be addressed independently.