Improves RBAC role naming conventions and increasing consistency.#1528
Improves RBAC role naming conventions and increasing consistency.#1528
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRole-name generation for EDA credentials now normalizes "Eda " to "EDA " when creating roles; a migration was added to rename existing role records to the new casing; tests were updated to assert the "EDA" casing for credential roles. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/aap_eda/core/migrations/0071_rename_eda_credential_roles.py`:
- Around line 55-62: The reverse migration loop over role_mappings
unconditionally renames RoleDefinition entries and can conflict if a role with
the target name already exists; update the loop in the migration that iterates
role_mappings to first check
RoleDefinition.objects.filter(name=new_name).exists() (or try to get new_name
and handle DoesNotExist) and if a target role exists, log a skipping message via
logger and do not perform the rename, otherwise perform the rename and save;
reference the role_mappings dict, the RoleDefinition model, and the existing
logger calls to mirror the forward migration's conflict guard.
In `@tests/integration/core/test_create_initial_data.py`:
- Around line 65-68: The assertions under the obj_name == "eda credential"
branch currently check lowercased names (role_names) so they don't verify the
intended EDA casing; change the test to assert against the original,
case-preserved role names (or stop lowercasing for this branch) and assert the
exact expected strings "EDA Credential Admin" and "EDA Credential Use" instead
of "eda credential admin"/"eda credential use" so the test actually validates
the EDA casing change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 302cc36f-e6b1-4f1e-85b8-b6ec0a29b1bc
📒 Files selected for processing (3)
src/aap_eda/core/management/commands/create_initial_data.pysrc/aap_eda/core/migrations/0071_rename_eda_credential_roles.pytests/integration/core/test_create_initial_data.py
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #1528 +/- ##
==========================================
- Coverage 91.93% 91.85% -0.09%
==========================================
Files 239 240 +1
Lines 10810 10850 +40
==========================================
+ Hits 9938 9966 +28
- Misses 872 884 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
AlexSCorey
force-pushed
the
59318-PermissionNamingConvention
branch
from
63ffb68 to
f9e928a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/aap_eda/core/migrations/0071_rename_eda_credential_roles.py (1)
82-85: Add coverage for migration skip branches (forward + reverse).This migration’s safety depends on the
already existsandnot foundbranches, and coverage indicates those paths are still partially untested. Please add migration tests for both branches in apply and rollback to reduce regression risk. As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/aap_eda/core/migrations/0071_rename_eda_credential_roles.py` around lines 82 - 85, Add tests for src/aap_eda/core/migrations/0071_rename_eda_credential_roles.py that exercise both conditional branches in rename_eda_credential_roles_forward and rename_eda_credential_roles_reverse: create one test that sets up the DB state so the target role already exists before applying the migration to hit the "already exists" branch on forward and ensure rollback handles it, and a second test that ensures the source role is missing before apply to hit the "not found" branch on forward and likewise verify the reverse handles the missing item; use Django's MigrationTestCase or the migration executor to run apply/rollback and set up/tear down role rows (or mock query results) to trigger each branch and assert expected SQL/ORM changes and no exceptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/aap_eda/core/migrations/0071_rename_eda_credential_roles.py`:
- Around line 82-85: Add tests for
src/aap_eda/core/migrations/0071_rename_eda_credential_roles.py that exercise
both conditional branches in rename_eda_credential_roles_forward and
rename_eda_credential_roles_reverse: create one test that sets up the DB state
so the target role already exists before applying the migration to hit the
"already exists" branch on forward and ensure rollback handles it, and a second
test that ensures the source role is missing before apply to hit the "not found"
branch on forward and likewise verify the reverse handles the missing item; use
Django's MigrationTestCase or the migration executor to run apply/rollback and
set up/tear down role rows (or mock query results) to trigger each branch and
assert expected SQL/ORM changes and no exceptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f089fb3c-7efb-41e8-909e-bb7738f4c15f
📒 Files selected for processing (3)
src/aap_eda/core/management/commands/create_initial_data.pysrc/aap_eda/core/migrations/0071_rename_eda_credential_roles.pytests/integration/core/test_create_initial_data.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/integration/core/test_create_initial_data.py
- src/aap_eda/core/management/commands/create_initial_data.py
|
Just to link this: The concern is that Customers will upgrade from prior 2.6.z versions to this. It's unclear if that will result in the desired behavior or not. It depends on the behavior of the sync from migrate_service_data, and I do not know this right now. |
|
Following advice from @AlanCoding I spun up a devel based aap-dev instance and found role names like |
|
Linking my conversation with @AlanCoding where he offers suggestions about how to test this pr. https://redhat-internal.slack.com/archives/C06CM09FQSV/p1776094482726909 |
AlexSCorey
force-pushed
the
59318-PermissionNamingConvention
branch
from
f9e928a to
ee9f7a1
Compare
|
5 participants
This addresses https://redhat.atlassian.net/browse/AAP-59318 partially. There are some reamining bits regarding rbac roles within awx, and possibly within DAB that would need to be addressed. For this PR the work was to improve the consistency of the RBAC role names.
Tests were also updated/added.
Summary by CodeRabbit
Bug Fixes
Tests