Upgrade DRF to 3.9.4 #3967
Upgrade DRF to 3.9.4 #3967
Conversation
|
Build failed.
|
|
Looking good so far 👍 |
|
Build succeeded.
|
| try: | ||
| from pip._internal.req import parse_requirements | ||
| except ImportError: | ||
| from pip.req import parse_requirements |
There was a problem hiding this comment.
Had to borrow this for my own branch
|
Build succeeded.
|
|
Build succeeded.
|
|
Build succeeded.
|
jbradberry
force-pushed
the
upgrade-drf-3.9.4
branch
from
1888245
to
28023db
Compare
|
Build failed.
|
|
Build succeeded.
|
|
Build failed.
|
|
recheck |
|
Build succeeded.
|
|
Build succeeded.
|
|
I tried out the branch and the links still aren't working
I do see some console errors, which seems to be because of mismatched jquery versions expected by DRF and what's actually there. I ran
|
|
It appears the thing I was missing was
I think I just didn't get it because of a check and egg dependency issue. |
| super().__init__(*args, **kwargs) | ||
|
|
||
| def to_representation(self, value): | ||
| fields = copy.deepcopy(self._declared_fields) |
There was a problem hiding this comment.
We've had some major problems with deepcopy() in the past, such that any time it's used the usage sticks out to me. It's probably fine? Are there deep references here that need the deep copy?
There was a problem hiding this comment.
This stuff is borrowed directly from DRF's implementation of Serializer, folded together a bit since we don't need the full generality. We could probably get away with not deepcopy-ing, since it seems unlikely that we'll dynamically add fields to these things, per https://github.com/encode/django-rest-framework/blob/master/rest_framework/serializers.py#L380-L383
| @@ -33,21 +33,23 @@ def test_internal_value_valid(self, data, expected): | |||
|
|
|||
| @pytest.mark.parametrize("data, expected", [ | |||
| ({'remove': 'blah', 'saml_attr': 'foobar'}, | |||
| ValidationError('"blah" is not a valid boolean.')), | |||
| {'remove': ['Must be a valid boolean.']}), | |||
There was a problem hiding this comment.
I'm commenting on the test, rather than the implementation.... but, it seems like we are losing some context here that we had before? Originally we were able to display what wasn't valid but we're moving to a more generic message?
There was a problem hiding this comment.
This one comes straight out of encode/django-rest-framework#5881 rather than anything explicit I did. The reasoning seems to be avoiding XSS attacks.
| automat==0.7.0 # via twisted | ||
| azure-common==1.1.20 # via azure-keyvault | ||
| azure-common==1.1.21 # via azure-keyvault |
There was a problem hiding this comment.
It's probably fine? But I noticed this and the pysocks requirement are getting updated here but seem unrelated to DRF?
There was a problem hiding this comment.
That's just due to following the instructions.
If we want to avoid unrelated dependency upgrades on feature changes, then we should consider a 2-track set of instructions there, where you run pip-compile -U when doing general upgrades, but without the upgrade flag when adding or removing a single entry. Even doing that, I'm not sure how nicely the tool would behave. It would need testing.
There was a problem hiding this comment.
I think it comes down to what we test here prior to merge. If we're upgrading these dependencies then we need to do a regression of the Tower components that are using them. In the cases I mentioned here it would have the to Credential Management services and Notifications(?). Those are normally manual tests... otherwise we should rewrite the requirements.[txt|in] to not include these updates quite yet.
There was a problem hiding this comment.
They must have been pulled in by my upgrade. I'll take a look to figure out why.
|
Build failed.
|
|
Build succeeded.
|
jbradberry
force-pushed
the
upgrade-drf-3.9.4
branch
from
1939b0f
to
796ed94
Compare
|
Build succeeded.
|
|
I agree with what Ryan said about the type changes. integer / object seems more correct than what it used to be. Looking over the last test results, it seems that "description" is missing specifically on the configure-tower-in-tower pages. That would be good to get sorted out. |
@AlanCoding agreed. I've been digging in to the missing descriptions and summaries, which must be caused by a pretty subtle change that is still mystifying me. |
|
Build succeeded.
|
|
@matburt @ryanpetrello @AlanCoding I now have 3 descriptions that are very verbose, but none missing. All other schema changes are the (mostly likely) harmless string -> int or string -> object changes. |
|
If you click the "?" button in those pages, what does it look like? For reference, here is a recent version of devel:
It trails off at "The following fields may be modified", which leads me to think this was a bug from the get-go. I'd just like to confirm how the new version looks. |
|
@AlanCoding hm, nice:
|
ryanpetrello
dismissed
matburt’s stale review
Looked at the remaining Swagger diffs and I think they're a non-issue.
It appeared to not be doing anything that we were making use of that couldn't already be done, slightly differently, using DRF's built-in one.
and fix the tests to handle the newer nested validation checks properly.
This is part 1 of the removal of awx/templates/rest_framework/base.html.
In the same manner as the current version of base.html from DRF. This is part 2 of the removal of base.html.
The staticfiles library will be going away before too long.
in order to correctly populate the default for the TOWER_URL_BASE field, which is specific to the host and preferred scheme.
jbradberry
force-pushed
the
upgrade-drf-3.9.4
branch
from
7969b92
to
aa54844
Compare
|
Build succeeded.
|
|
Build succeeded (gate pipeline).
|
fix a typo in the CLI usage docs
SUMMARY
Upgrade Django REST Framework from our current requirement of 3.7.7 to the latest available version, 3.9.4. This is part of the overall upgrade of Django to the latest LTS version, 2.2.
related #2863
removal of rest_framework/base.html
It is a pain point to have to backport changes that DRF makes to their copy of the template into our version, and most of our changes have had fixes on their side already. Those things that are specific to AWX were moved to block overrides in rest_framework/api.html.
refactor of the SSO settings serializer fields
DRF has changed the validation behavior for nested fields, among other things. The base field class that we use now,
HybridDictField, has been rewritten to be more declarative and make more use of DRF's existing machinery.removal of the custom
get_view_namefunctionIt was apparently originally put in place to support functionality that DRF has since added to the default version of the function. Supporting the DRF version only involved changing each view's
view_nameattribute toname.modifications to the
get_view_descriptionfunctionalitya fix to a regression around Swagger schema production and metadata for the settings-in-db functionality
ISSUE TYPE
COMPONENT NAME
AWX VERSION