8000 Resolve default ordering warnings from tests by AlanCoding · Pull Request #3820 · ansible/awx · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Resolve default ordering warnings from tests #3820

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 20, 2019
Merged

Resolve default ordering warnings from tests #3820

merged 1 commit into from
May 20, 2019

Conversation

AlanCoding
Copy link
Member
SUMMARY

The tests were giving warnings

awx/main/tests/functional/api/test_organizations.py::test_organization_user_list
  /venv/awx/lib64/python3.6/site-packages/rest_framework/pagination.py:208: UnorderedObjectListWarning: Pagination may yield inconsistent results with an unordered object_list: <class 'django.contrib.auth.models.User'> QuerySet.
    paginator = self.django_paginator_class(queryset, page_size)

There were a lot of them. After this there should be none.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME
  • API
AWX VERSION
4.0.0
ADDITIONAL INFORMATION

In some cases, we can't modify the Meta of the model, because it is not our model.

Because of that, I borrowed the closest thing to the standard pattern from DRF:

https://github.com/encode/django-rest-framework/blob/14fad0d6902550cd61fc07370759ef785effa207/rest_framework/filters.py#L186-L190

So when you see the ordering parameter set on a view, you should know that I did not invent that.

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

rooftopcellist
rooftopcellist approved these changes May 3, 2019
View reviewed changes
Copy link
Member
@rooftopcellist rooftopcellist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this! All of the order_by's make sense.

rooftopcellist
rooftopcellist reviewed May 3, 2019
View reviewed changes
awx/api/views/__init__.py
@@ -4448,6 +4452,7 @@ class RoleUsersList(SubListAttachDetachAPIView):
serializer_class = serializers.UserSerializer
parent_model = models.Role
relationship = 'members'
ordering = ('username',)
Copy link
Member
@rooftopcellist rooftopcellist May 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could alternatively do this on the model, which would be more concise.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree entirely for the OAuth2 models, because we maintain those. Pushed a commit that does this.

But we don't have a User model inside of our app, so I want to keep these for that model, and only that model.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@AlanCoding
Copy link
Member Author

Blocked, as the migration claim is not settled

@ryanpetrello
Copy link
Contributor

This is probably unblocked once something like #3842 is merged.

@ryanpetrello
8000 Copy link
Contributor

Not blocked anymore 👀

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@AlanCoding
Copy link
Member Author

Got test output, and only remaining warning is:

awx-manage check_migrations --dry-run --check  -n 'vNNN_missing_migration_file'
No changes detected
Exception TypeError: "'NoneType' object is not callable" in <function _removeHandlerRef at 0x7faefe528ed8> ignored
api finish: runtests after 224.29 seconds
api start: run-test-post 
api finish: run-test-post after 0.00 seconds

From some googling on this problem, it might be platform-specific, and anyway, it's probably harmless and related to the python stdlib logging.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit 29bbecb into ansible:devel May 20, 2019
ryanpetrello pushed a commit to ryanpetrello/awx that referenced this pull request Oct 29, 2019
@AlanCoding @ryanpetrello
Add support for credential_type in tower_credential module (ansible#3820
30a0b05 
)

* Add support for credential_type

* Finish up credential_type parameter with tests

* make inputs mutually exclusive with other params

* Test credential type with dict input
ryanpetrello pushed a commit to ryanpetrello/awx that referenced this pull request Oct 29, 2019
@AlanCoding @ryanpetrello
Add support for credential_type in tower_credential module (ansible#3820
70269d9 
)

* Add support for credential_type

* Finish up credential_type parameter with tests

* make inputs mutually exclusive with other params

* Test credential type with dict input
AlanCoding added a commit to AlanCoding/awx that referenced this pull request Oct 29, 2019
@AlanCoding
Add support for credential_type in tower_credential module (ansible#3820
fb67b8e 
)

* Add support for credential_type

* Finish up credential_type parameter with tests

* make inputs mutually exclusive with other params

* Test credential type with dict input
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rooftopcellist rooftopcellist rooftopcellist approved these changes

Assignees
No one assigned
Labels
component:api type:bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AlanCoding @ryanpetrello @rooftopcellist @awxbot
0