Skip to content

Resolve default ordering warnings from tests#3820

Merged
softwarefactory-project-zuul[bot] merged 1 commit into
ansible:develfrom
AlanCoding:default_ordering
May 20, 2019
Merged

Resolve default ordering warnings from tests#3820
softwarefactory-project-zuul[bot] merged 1 commit into
ansible:develfrom
AlanCoding:default_ordering

Conversation

@AlanCoding

Copy link
Copy Markdown
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
Copy Markdown
Contributor

Build failed.

@softwarefactory-project-zuul

Copy link
Copy Markdown
Contributor

Build succeeded.

@rooftopcellist rooftopcellist left a comment

Copy link
Copy Markdown
Member

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.

Comment thread awx/api/views/__init__.py
serializer_class = serializers.UserSerializer
parent_model = models.Role
relationship = 'members'
ordering = ('username',)

@rooftopcellist rooftopcellist May 3, 2019

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
Contributor

Build succeeded.

@AlanCoding

Copy link
Copy Markdown
Member Author

Blocked, as the migration claim is not settled

@ryanpetrello

Copy link
Copy Markdown
Contributor

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

@ryanpetrello

Copy link
Copy Markdown
Contributor

Not blocked anymore 👀

@softwarefactory-project-zuul

Copy link
Copy Markdown
Contributor

Build failed.

@softwarefactory-project-zuul

Copy link
Copy Markdown
Contributor

Build succeeded.

@AlanCoding

Copy link
Copy Markdown
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
Copy Markdown
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
)

* 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
)

* 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
)

* 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants