-
Notifications
You must be signed in to change notification settings - Fork 3.6k
AAP-64432 Fix wrong field rename for github app #16271
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.
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
AAP-64432 Fix wrong field rename for github app #16271
Conversation
|
Tested in my dev env manually:
|
7de522c to
c9d69bd
Compare
📝 WalkthroughWalkthroughAdds no-op reverse callables to two RunPython migrations, fixes a CredentialType migration to use Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@awx/main/tests/functional/test_migrations.py`:
- Around line 186-191: Update the stale comments and docstring in the
test_after_github_app_kind_migration test to reference the correct migration
name and field: change all occurrences of "0202_squashed_deletions" to
"0204_squashed_deletions" and replace references to the updated field "kind"
with "namespace" (or similar wording reflecting the actual change). Ensure any
inline comments elsewhere in the same test file that mention the migration
number or the field update (including comments around assertions and setup in
test_migrations.py) are adjusted so they accurately describe that
update_github_app_kind changed the namespace field after the
0204_squashed_deletions migration.
- Around line 217-220: The assertion is vacuous because the test created
CredentialType(kind='external'), so checking that no CredentialType with
kind='github_app' exists is meaningless; update the assertion to verify that no
CredentialType with namespace='github_app' exists after the migration (e.g.
replace the filter kind='github_app' with namespace='github_app' against
CredentialType.objects.exists()) so the test asserts the migration removed the
namespace value as intended (refer to the CredentialType model and the test
function in test_migrations.py).
🧹 Nitpick comments (1)
awx/main/migrations/0204_squashed_deletions.py (1)
7-15: Stale docstring — still referenceskindfield.The docstring says "Updates the 'kind' field" but the code now updates
namespace. The function nameupdate_github_app_kindis similarly misleading. Consider updating at least the docstring to reflect the actual change. Renaming the function is fine too since it's only referenced within this migration file (Line 122).Suggested docstring fix
def update_github_app_kind(apps, schema_editor): """ - Updates the 'kind' field for CredentialType records - from 'github_app' to 'github_app_lookup'. - This addresses a change in the entry point key for the GitHub App plugin. + Updates the 'namespace' field for CredentialType records + from 'github_app' to 'github_app_lookup'. + This addresses a change in the entry point key for the GitHub App plugin. """
c9d69bd to
7ec0705
Compare
|
@coderabbitai review |
* Multiple credentialtype's have the same kind and kind values look like: cloud, network, machine, etc. * namespace is the field that we want to rename
7ec0705 to
825f5d0
Compare
* Introduced in PR https://github.com/ansible/awx/pull/16058/changes then a later large merge from AAP back into devel removed the changes * This PR re-introduces the github app lookup migration rename tests with the migration names updated and the kind to namespace correction
825f5d0 to
7799104
Compare
|
fosterseth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see a couple of references in aap-ui that might need to be updated



Jira: https://issues.redhat.com/browse/AAP-64432
SUMMARY
ISSUE TYPE
COMPONENT NAME
ADDITIONAL INFORMATION
Note
Low Risk
Migration-only changes with a targeted data update and added tests; risk is limited to upgrade paths where
CredentialType.namespacevalues are rewritten.Overview
Fixes the GitHub App credential-type migration in
0204_squashed_deletionsto updateCredentialType.namespacefromgithub_apptogithub_app_lookup(instead of incorrectly updatingkind).Makes several
RunPythonmigrations explicitly reversible in tests by addingmigrations.RunPython.noopreverse functions (in0201_create_managed_creds,0202_convert_controller_role_definitions, and the GitHub App update step). Adds a functional migration regression test that creates a legacynamespace='github_app'record, applies0204, and asserts the value is rewritten to prevent thecreatesuperuserKeyError.Written by Cursor Bugbot for commit 7799104. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Bug Fixes
Chores
Tests