[WIP] [AAP-54064] Decoupling apps from ansible_base.rbac #849
Conversation
john-westcott-iv
changed the title
john-westcott-iv
force-pushed
the
AAP-54064-2
branch
from
c85d394 to
97d5317
Compare
|
john-westcott-iv
force-pushed
the
AAP-54064-2
branch
2 times, most recently
from
cd97257 to
302d51e
Compare
john-westcott-iv
force-pushed
the
AAP-54064-2
branch
from
302d51e to
fcc27e5
Compare
|
DVCS PR Check Results: PR appears valid (JIRA key(s) found) |
| User = get_user_model() | ||
|
|
||
|
|
||
| is_rbac_installed = 'ansible_base.rbac' in settings.INSTALLED_APPS |
There was a problem hiding this comment.
I really want to stop doing this referencing of settings. on import. It's an import circularity problem, and here you don't need it anyway. You can make this into a method.
There was a problem hiding this comment.
Yea we do this all over. We might want to just have a generic method like is_dab_app_installed('rbac')
| _service_id = str(service_obj.pk) | ||
| else: | ||
| # Create a ServiceID if none exists | ||
| service_obj = ServiceID.objects.create() |
There was a problem hiding this comment.
Oh, this is a major thing I had on my agenda.
https://issues.redhat.com/browse/AAP-51352
I had assumed we couldn't do it like this. But I'm not saying no.
| "shared.user": ResourceTypeProcessor, | ||
| } | ||
| if 'ansible_base.rbac' in settings.INSTALLED_APPS: | ||
| processors["shared.roledefinition"] = RoleDefinitionProcessor |
There was a problem hiding this comment.
👍 agreed.
This should cause no ill-effects. For the major services, we simply don't care about the case that the RBAC app is not installed.
There was a problem hiding this comment.
I somewhat disagree with this. There is no problem with making requests to the RBAC related endpoints when RBAC is not installed locally.
Some methods are still invalid to call, but not all of these.
| def to_internal_value(self, data): | ||
| if 'ansible_base.rbac' not in settings.INSTALLED_APPS: | ||
| raise RuntimeError("LenientPermissionSlugListField requires ansible_base.rbac to be installed") | ||
| from ansible_base.rbac.models import DABPermission |
There was a problem hiding this comment.
this seems entirely unnecessary. The rest of changes in this file, yes.
|
|
||
| logger = logging.getLogger('ansible_base.resources_api.tasks.sync') | ||
|
|
||
| _is_rbac_installed = 'ansible_base.rbac' in settings.INSTALLED_APPS |
There was a problem hiding this comment.
Here too, I want to bring this in-line.
john-westcott-iv
changed the title
|
I put my review comments in john-westcott-iv#11 |
Description
What is being changed?
Attempting to decouple apps from the rbac app.
Why is this change needed?
Devs may want to include apps w/o including the rbac app.
How does this change address the issue?
Removes direct import from ansible_base.rbac but keeps functionally by checking if ansible_base.rbac is installed.
Type of Change
Self-Review Checklist
Testing Instructions
Prerequisites
Steps to Test
Expected Results
Additional Context
Required Actions
Screenshots/Logs