Address more ignored pytest warnings, co-authored with Opus 4.6#16298
Address more ignored pytest warnings, co-authored with Opus 4.6#16298AlanCoding wants to merge 3 commits intoansible:develfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAdded default primary-key ordering to three Django model Meta classes (InstanceGroup, WorkflowJobNode, WorkflowJobTemplateNode), added a migration to apply those options, and simplified pytest.ini by removing multiple specific filterwarnings and updating JUnit reporting settings. 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
awx/main/models/workflow.py (1)
197-203:⚠️ Potential issue | 🟠 MajorA corresponding migration file is required for
Meta.orderingchanges.Django's migration framework tracks
Meta.orderingvia theAlterModelOptionsoperation. Addingordering = ('pk',)to bothWorkflowJobTemplateNode.Meta(line 203) andWorkflowJobNode.Meta(line 290) requires a migration file. AWX's CI pipeline includes an explicitapi-migrationstest job that runs all migrations and will fail if unapplied migrations are detected. A migration file must be generated and committed alongside these model changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx/main/models/workflow.py` around lines 197 - 203, The change adds Meta.ordering = ('pk',) to both WorkflowJobTemplateNode.Meta and WorkflowJobNode.Meta but no migration was committed; run Django makemigrations to generate an explicit migration that contains an AlterModelOptions for these two models (or hand-write an operation that sets ordering if preferred), add and commit the generated migration file to the repo alongside the model changes so the api-migrations CI job will apply it.
🧹 Nitpick comments (1)
awx/main/models/workflow.py (1)
203-203: Minor inconsistency: use'id'to match the existing convention in this file.
WorkflowJob.Meta(line 642) usesordering = ('id',). Both'pk'and'id'resolve identically, but aligning to'id'keeps the file internally consistent.♻️ Proposed change
- ordering = ('pk',) # WorkflowJobTemplateNode.Meta line 203 + ordering = ('id',) - ordering = ('pk',) # WorkflowJobNode.Meta line 290 + ordering = ('id',)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx/main/models/workflow.py` at line 203, Change the Meta ordering tuple in the Workflow model from ('pk',) to ('id',) to match the file-wide convention (e.g., as used in WorkflowJob.Meta); locate the ordering = ('pk',) line inside the Workflow.Meta class and replace 'pk' with 'id' so the file consistently uses 'id' for model ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@awx/main/models/workflow.py`:
- Around line 197-203: The change adds Meta.ordering = ('pk',) to both
WorkflowJobTemplateNode.Meta and WorkflowJobNode.Meta but no migration was
committed; run Django makemigrations to generate an explicit migration that
contains an AlterModelOptions for these two models (or hand-write an operation
that sets ordering if preferred), add and commit the generated migration file to
the repo alongside the model changes so the api-migrations CI job will apply it.
---
Nitpick comments:
In `@awx/main/models/workflow.py`:
- Line 203: Change the Meta ordering tuple in the Workflow model from ('pk',) to
('id',) to match the file-wide convention (e.g., as used in WorkflowJob.Meta);
locate the ordering = ('pk',) line inside the Workflow.Meta class and replace
'pk' with 'id' so the file consistently uses 'id' for model ordering.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@awx/main/models/ha.py`:
- Around line 486-491: The InstanceGroup.Meta ordering was changed to ordering =
('pk',) but no Django migration was added; create a new migration that uses
migrations.AlterModelOptions to record the ordering change for the InstanceGroup
model (similar to existing migrations like 0077_v360_add_default_orderings.py or
0191_add_django_permissions.py), and if you made a parallel Meta.ordering change
in any workflow-related model (e.g., in workflow.py), include an
AlterModelOptions entry for that model as well; ensure the migration module is
named/numbered consistently, imports the correct app label 'main', and only
alters the model options (no schema changes).
| class Meta: | ||
| app_label = 'main' | ||
| ordering = ('pk',) | ||
| permissions = [('use_instancegroup', 'Can use instance group in a preference list of a resource')] | ||
| # Since this has no direct organization field only superuser can add, so remove add permission | ||
| default_permissions = ('change', 'delete', 'view') |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for migrations referencing InstanceGroup ordering
rg -n "ordering.*pk" awx/main/migrations/ --type py -lRepository: ansible/awx
Length of output: 257
🏁 Script executed:
#!/bin/bash
# List all migration files in awx/main/migrations to understand recent changes
ls -la awx/main/migrations/ | tail -20Repository: ansible/awx
Length of output: 1428
🏁 Script executed:
#!/bin/bash
# Search for InstanceGroup migrations more broadly
rg -n "InstanceGroup" awx/main/migrations/ --type py -l | head -10Repository: ansible/awx
Length of output: 574
🏁 Script executed:
#!/bin/bash
# Find the highest numbered migration file to see what's most recent
ls -1 awx/main/migrations/*.py | grep -E '0[0-9]{3}_' | sort -V | tail -5Repository: ansible/awx
Length of output: 316
🏁 Script executed:
#!/bin/bash
# Search all migrations for any mention of ordering on InstanceGroup
rg -n "InstanceGroup" awx/main/migrations/*.py -A 5 -B 5 | grep -A 10 -B 10 "ordering"Repository: ansible/awx
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Check git status to see if there are new migration files in this PR
git status --short awx/main/migrations/ 2>/dev/null | head -20Repository: ansible/awx
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Check the actual changes to ha.py - is ordering new or already existed?
git diff HEAD awx/main/models/ha.py 2>/dev/null | head -100Repository: ansible/awx
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Look at the current InstanceGroup model to confirm ordering is in place
rg -A 10 "class InstanceGroup" awx/main/models/ha.py | head -30Repository: ansible/awx
Length of output: 562
🏁 Script executed:
#!/bin/bash
# Check if workflow.py also has ordering changes
git diff HEAD awx/main/models/workflow.py 2>/dev/null | head -100Repository: ansible/awx
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Read the InstanceGroup Meta class directly from ha.py (lines 480-495)
sed -n '480,495p' awx/main/models/ha.pyRepository: ansible/awx
Length of output: 641
🏁 Script executed:
#!/bin/bash
# Check if there's a recent migration that might include the ordering change
find awx/main/migrations -name "*.py" -type f -exec grep -l "ordering" {} \; | sortRepository: ansible/awx
Length of output: 803
🏁 Script executed:
#!/bin/bash
# Look for any migration files that might be uncommitted or new in this context
git log --oneline awx/main/migrations/ 2>/dev/null | head -10Repository: ansible/awx
Length of output: 96
🏁 Script executed:
#!/bin/bash
# Check the content of the ordering migration to see if it includes InstanceGroup
cat awx/main/migrations/0077_v360_add_default_orderings.pyRepository: ansible/awx
Length of output: 1987
🏁 Script executed:
#!/bin/bash
# Also check if workflow.py has ordering in its Meta class
rg -A 8 "class.*Meta:" awx/main/models/workflow.py | grep -A 8 "ordering"Repository: ansible/awx
Length of output: 657
🏁 Script executed:
#!/bin/bash
# Check if there's a migration that altered InstanceGroup options more recently
rg "InstanceGroup" awx/main/migrations/020*.py -A 3 -B 3Repository: ansible/awx
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Verify the commit history to see when ordering was added to InstanceGroup
git log --all --oneline --follow -S "ordering = ('pk',)" -- awx/main/models/ha.py 2>/dev/null | head -5Repository: ansible/awx
Length of output: 141
🏁 Script executed:
#!/bin/bash
# List all migration files numbered 200 or higher
ls -1 awx/main/migrations/02*.py | sort -VRepository: ansible/awx
Length of output: 316
🏁 Script executed:
#!/bin/bash
# Check the latest migrations for AlterModelOptions on InstanceGroup
rg "AlterModelOptions" awx/main/migrations/*.py | grep -i instanceRepository: ansible/awx
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Check if ordering was in the model before (search initial/squashed migrations)
rg "ordering.*=.*pk" awx/main/migrations/0001_initial.py -A 2 -B 2Repository: ansible/awx
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Check the squashed migrations for InstanceGroup ordering
rg "InstanceGroup" awx/main/migrations/0002_squashed* awx/main/migrations/0003_squashed* awx/main/migrations/0004_squashed* -A 5 | grep -A 5 "ordering"Repository: ansible/awx
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Look at migration 0204 (latest squashed) to see what it covers
head -30 awx/main/migrations/0204_squashed_deletions.pyRepository: ansible/awx
Length of output: 1326
🏁 Script executed:
#!/bin/bash
# Search for when ordering was added to InstanceGroup by checking all migrations
for file in awx/main/migrations/0*.py; do if grep -q "InstanceGroup" "$file" && grep -q "ordering" "$file"; then echo "=== $file ==="; grep -A 10 "InstanceGroup" "$file" | grep -A 3 "ordering"; fi; doneRepository: ansible/awx
Length of output: 103
🏁 Script executed:
#!/bin/bash
# Double-check: search all migration files for any reference to InstanceGroup with ordering operations
grep -r "InstanceGroup" awx/main/migrations/*.py | grep -i "options\|ordering"Repository: ansible/awx
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Check if ordering was already in InstanceGroup by looking at the squashed initial migrations
grep -A 20 "class InstanceGroup" awx/main/migrations/0001_initial.py | head -25Repository: ansible/awx
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Verify workflow.py models also lack migrations for ordering
rg "class.*Meta:" awx/main/models/workflow.py -B 10 | grep "class " | head -5Repository: ansible/awx
Length of output: 214
🏁 Script executed:
#!/bin/bash
# Check if there are any .py files in migrations directory that are uncommitted or new
git status awx/main/migrations/ 2>/dev/null || echo "Git status not available"Repository: ansible/awx
Length of output: 120
🏁 Script executed:
#!/bin/bash
# Explicitly check 0077 migration to confirm InstanceGroup is NOT included
grep -n "InstanceGroup\|instance_group" awx/main/migrations/0077_v360_add_default_orderings.pyRepository: ansible/awx
Length of output: 121
🏁 Script executed:
#!/antml:bash
# Final check: list all AlterModelOptions operations across ALL migrations
rg "AlterModelOptions" awx/main/migrations/*.py -B 2 -A 2 | grep -E "(name=|ordering)" | head -30Repository: ansible/awx
Length of output: 3123
🏁 Script executed:
#!/an/bash
# Check what the InstanceGroup AlterModelOptions in 0191 actually contains
sed -n '/instancegroup/,/^ \)/p' awx/main/migrations/0191_add_django_permissions.py | head -20Repository: ansible/awx
Length of output: 104
🏁 Script executed:
#!/antml/bash
# Get the full context around InstanceGroup in 0191_add_django_permissions.py
rg -A 5 "name='instancegroup'" awx/main/migrations/0191_add_django_permissions.pyRepository: ansible/awx
Length of output: 583
A migration for the ordering change is missing from this PR.
The ordering = ('pk',) addition to InstanceGroup.Meta (line 488) is present in the model code but has no corresponding Django migration. Django tracks all Meta.ordering changes in the migration state, even when they produce no SQL schema changes. Create a new migration to document this model state change, similar to existing migrations like 0077_v360_add_default_orderings.py or 0191_add_django_permissions.py that use AlterModelOptions for InstanceGroup. Apply the same requirement to any corresponding changes in workflow.py.
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 489-489: Mutable default value for class attribute
(RUF012)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@awx/main/models/ha.py` around lines 486 - 491, The InstanceGroup.Meta
ordering was changed to ordering = ('pk',) but no Django migration was added;
create a new migration that uses migrations.AlterModelOptions to record the
ordering change for the InstanceGroup model (similar to existing migrations like
0077_v360_add_default_orderings.py or 0191_add_django_permissions.py), and if
you made a parallel Meta.ordering change in any workflow-related model (e.g., in
workflow.py), include an AlterModelOptions entry for that model as well; ensure
the migration module is named/numbered consistently, imports the correct app
label 'main', and only alters the model options (no schema changes).
|
1 participant
SUMMARY
Same as #16269
But fed the same prompt to it again. Addressing low-hanging fruit.
ISSUE TYPE
COMPONENT NAME
Note
Low Risk
Model
Meta.orderingchanges and test warning filter tweaks are low-impact; primary risk is subtle changes in default API/list ordering where callers relied on implicit DB ordering.Overview
Ensures deterministic queryset ordering by default by setting
ordering = ('pk',)forInstanceGroup,WorkflowJobNode, andWorkflowJobTemplateNode(plus a new Django migration to apply the model option changes).Cleans up
pytest.iniwarning filters by removing no-longer-needed suppressions and adding a targeted ignore forUnorderedObjectListWarningemitted by unorderedResourcepagination.Written by Cursor Bugbot for commit cbcbf4c. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Chores
Tests