Add opa_query_path to collection modules for inventory and job template#16138
Open
derekwaters wants to merge 6 commits intoansible:develfrom
Open
Add opa_query_path to collection modules for inventory and job template#16138derekwaters wants to merge 6 commits intoansible:develfrom
derekwaters wants to merge 6 commits intoansible:develfrom
Conversation
github-actions
Bot
added
component:awx_collection
derekwaters
force-pushed
the
add_opa_query_path_to_collection
branch
2 times, most recently
from
f42974e to
865b6fb
Compare
derekwaters
force-pushed
the
add_opa_query_path_to_collection
branch
from
865b6fb to
557d520
Compare
TheRealHaoLiu
force-pushed
the
add_opa_query_path_to_collection
branch
from
557d520 to
8eca7bf
Compare
derekwaters
force-pushed
the
add_opa_query_path_to_collection
branch
from
b286741 to
38a206d
Compare
derekwaters
force-pushed
the
add_opa_query_path_to_collection
branch
from
38a206d to
f43384b
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an optional string parameter 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
awx_collection/test/awx/test_inventory.py (1)
20-20:opa_query_pathadded totest_inventory_createargs but never asserted in that test.Since
test_inventory_create_with_opaalready coversopa_query_pathstorage, either add the assertion here or remove the field from this test'smodule_argsto avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx_collection/test/awx/test_inventory.py` at line 20, The test_inventory_create includes 'opa_query_path' in module_args but never asserts it; either remove 'opa_query_path' from the module_args in test_inventory_create or add an assertion that the created inventory stores it (mirror the check used in test_inventory_create_with_opa). Locate test_inventory_create and its module_args and either delete the 'opa_query_path': 'foo/bar' entry or add an assertion (e.g., assert the returned/created inventory's opa_query_path equals 'foo/bar') consistent with how test_inventory_create_with_opa verifies storage.
🤖 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_collection/plugins/modules/job_template.py`:
- Around line 318-321: The description for the parameter opa_query_path
incorrectly mentions "Inventory"; update the string in the job_template module
so the description references "Job Template" instead—locate the opa_query_path
parameter definition in awx_collection/plugins/modules/job_template.py and
change the sentence "to an OPA policy to be applied to the Inventory" to "to an
OPA policy to be applied to the Job Template" (preserve formatting and type:
str).
In `@awx_collection/plugins/modules/organization.py`:
- Around line 87-90: The description for the opa_query_path parameter
incorrectly references "Inventory" instead of "Organization"; update the
description string for opa_query_path in
awx_collection/plugins/modules/organization.py to say "Organization" (e.g., "The
optional path (formatted as package/rule) to an OPA policy to be applied to the
Organization") so the parameter documentation correctly targets Organization.
---
Nitpick comments:
In `@awx_collection/test/awx/test_inventory.py`:
- Line 20: The test_inventory_create includes 'opa_query_path' in module_args
but never asserts it; either remove 'opa_query_path' from the module_args in
test_inventory_create or add an assertion that the created inventory stores it
(mirror the check used in test_inventory_create_with_opa). Locate
test_inventory_create and its module_args and either delete the
'opa_query_path': 'foo/bar' entry or add an assertion (e.g., assert the
returned/created inventory's opa_query_path equals 'foo/bar') consistent with
how test_inventory_create_with_opa verifies storage.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
awx_collection/test/awx/test_inventory.py (1)
20-28: Consider assertingopa_query_pathwas persisted.The test sends
opa_query_pathbut doesn't verify it was saved to the inventory. Once the module bug is fixed, add an assertion to confirm the value is persisted.💡 Suggested assertion
inv = Inventory.objects.get(name='foo-inventory') assert inv.variables == '{"foo": "bar", "another-foo": {"barz": "bar2"}}' + assert inv.opa_query_path == 'foo/bar'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx_collection/test/awx/test_inventory.py` around lines 20 - 28, Add an assertion that the sent opa_query_path was persisted: after retrieving the Inventory instance (inv = Inventory.objects.get(name='foo-inventory')), assert that inv.opa_query_path == 'foo/bar' (or the exact persisted representation used by the Inventory model) to verify the module saved the value.
🤖 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_collection/plugins/modules/inventory.py`:
- Line 176: The parameter opa_query_path is read from module.params but never
added to the payload dictionary, so the API never receives it; update the
inventory payload assembly (the inventory_fields dict used when
creating/updating inventories) to include the opa_query_path key when
module.params contains a value (same pattern used for other fields in this file
and as in job_template.py/organization.py), and apply the same addition in the
other payload-building blocks around the 201-212 area so the field is sent on
both create and update flows.
In `@awx_collection/test/awx/test_inventory.py`:
- Around line 82-86: Remove the unresolved Git merge conflict markers (<<<<<<<,
=======, >>>>>>>) in the test file and keep a single correct assertion line (the
assert comparing result to {"name": "foo-inventory-opa", "id": inv.id,
"changed": True}); ensure only one assert remains and no conflict markers are
left so the test function that uses result and inv runs with valid Python
syntax.
---
Nitpick comments:
In `@awx_collection/test/awx/test_inventory.py`:
- Around line 20-28: Add an assertion that the sent opa_query_path was
persisted: after retrieving the Inventory instance (inv =
Inventory.objects.get(name='foo-inventory')), assert that inv.opa_query_path ==
'foo/bar' (or the exact persisted representation used by the Inventory model) to
verify the module saved the value.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
awx_collection/plugins/modules/inventory.pyawx_collection/test/awx/test_inventory.py
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_collection/test/awx/test_inventory.py (1)
20-29:⚠️ Potential issue | 🔴 CriticalTest will always fail due to value mismatch.
Line 20 sets
opa_query_pathto'foo/bar', but line 29 asserts it equals'foo/baz'. This inconsistency will cause the test to fail.🐛 Proposed fix
'name': 'foo-inventory', 'organization': organization.name, 'variables': {'foo': 'bar', 'another-foo': {'barz': 'bar2'}}, - 'opa_query_path': 'foo/bar', + 'opa_query_path': 'foo/baz', 'state': 'present',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx_collection/test/awx/test_inventory.py` around lines 20 - 29, The test sets opa_query_path to 'foo/bar' but asserts inv.opa_query_path == 'foo/baz', causing a guaranteed failure; fix by making the values consistent—either change the input in the inventory creation call to use 'opa_query_path': 'foo/baz' or update the assertion to expect 'foo/bar' (refer to the inventory creation call where Inventory.objects.get(name='foo-inventory') is used and the inv.opa_query_path assertion).
🤖 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_collection/test/awx/test_inventory.py`:
- Around line 20-29: The test sets opa_query_path to 'foo/bar' but asserts
inv.opa_query_path == 'foo/baz', causing a guaranteed failure; fix by making the
values consistent—either change the input in the inventory creation call to use
'opa_query_path': 'foo/baz' or update the assertion to expect 'foo/bar' (refer
to the inventory creation call where Inventory.objects.get(name='foo-inventory')
is used and the inv.opa_query_path assertion).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
awx_collection/plugins/modules/inventory.pyawx_collection/plugins/modules/job_template.pyawx_collection/plugins/modules/organization.pyawx_collection/test/awx/test_inventory.py
🚧 Files skipped from review as they are similar to previous changes (3)
- awx_collection/plugins/modules/organization.py
- awx_collection/plugins/modules/inventory.py
- awx_collection/plugins/modules/job_template.py
derekwaters
force-pushed
the
add_opa_query_path_to_collection
branch
from
f4791cf to
7b169d5
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
awx_collection/test/awx/test_inventory.py (1)
64-83: Consider de-duplicating overlapping OPA create tests.
test_inventory_createnow already validates create +opa_query_pathpersistence, so this new test largely repeats the same path. Keeping one focused OPA create test and one baseline non-OPA create test would reduce maintenance churn.Optional simplification
-@pytest.mark.django_db -def test_inventory_create_with_opa(run_module, admin_user, organization): - result = run_module( - 'inventory', - { - 'name': 'foo-inventory-opa', - 'organization': organization.name, - 'opa_query_path': 'foo/baz', - 'state': 'present', - }, - admin_user, - ) - assert not result.get('failed', False), result.get('msg', result) - - inv = Inventory.objects.get(name='foo-inventory-opa') - assert inv.opa_query_path == 'foo/baz' - - result.pop('module_args', None) - result.pop('invocation', None) - assert result == {"name": "foo-inventory-opa", "id": inv.id, "changed": True}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx_collection/test/awx/test_inventory.py` around lines 64 - 83, The new test test_inventory_create_with_opa duplicates coverage already provided by test_inventory_create; consolidate by keeping one focused OPA-specific test and one baseline create test: either remove test_inventory_create_with_opa and enhance test_inventory_create to assert both non-OPA and OPA behaviors in separate cases, or change test_inventory_create_with_opa to only assert OPA-specific behavior (e.g., only verify opa_query_path persistence and related OPA logic) while ensuring test_inventory_create only verifies baseline create semantics (name, id, changed) without asserting opa_query_path; update or delete the redundant assertions accordingly in the test functions to avoid overlap.awx_collection/plugins/modules/organization.py (1)
211-212: Add safeguards or documentation foropa_query_pathAAP version compatibility.The module writes
opa_query_pathunconditionally to the organization endpoint without version checks or capability guards. If this field is read-only or unsupported on specific AAP 2.5 deployments, failures will occur silently. Either add version/capability checks before writing (similar to the base module's version handling) or explicitly document AAP version constraints in the module docstring, following the pattern used forcustom_virtualenv.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx_collection/plugins/modules/organization.py` around lines 211 - 212, The code unconditionally sets org_fields['opa_query_path'] when opa_query_path is provided; add the same AAP version/capability guard used elsewhere (or document the constraint) so we don't send unsupported/read-only fields to older AAP 2.5 endpoints. Update organization.py to either: a) check the controller/version capability before assigning opa_query_path to org_fields (mirror the pattern used for custom_virtualenv/version checks in the base module) and skip or raise a clear error if unsupported, or b) add docstring/docs noting the minimum AAP version that supports opa_query_path; reference the opa_query_path variable, org_fields dict, and the organization module's version-handling helper functions to implement the guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@awx_collection/plugins/modules/organization.py`:
- Around line 211-212: The code unconditionally sets
org_fields['opa_query_path'] when opa_query_path is provided; add the same AAP
version/capability guard used elsewhere (or document the constraint) so we don't
send unsupported/read-only fields to older AAP 2.5 endpoints. Update
organization.py to either: a) check the controller/version capability before
assigning opa_query_path to org_fields (mirror the pattern used for
custom_virtualenv/version checks in the base module) and skip or raise a clear
error if unsupported, or b) add docstring/docs noting the minimum AAP version
that supports opa_query_path; reference the opa_query_path variable, org_fields
dict, and the organization module's version-handling helper functions to
implement the guard.
In `@awx_collection/test/awx/test_inventory.py`:
- Around line 64-83: The new test test_inventory_create_with_opa duplicates
coverage already provided by test_inventory_create; consolidate by keeping one
focused OPA-specific test and one baseline create test: either remove
test_inventory_create_with_opa and enhance test_inventory_create to assert both
non-OPA and OPA behaviors in separate cases, or change
test_inventory_create_with_opa to only assert OPA-specific behavior (e.g., only
verify opa_query_path persistence and related OPA logic) while ensuring
test_inventory_create only verifies baseline create semantics (name, id,
changed) without asserting opa_query_path; update or delete the redundant
assertions accordingly in the test functions to avoid overlap.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (6)
awx_collection/plugins/modules/inventory.pyawx_collection/plugins/modules/job_template.pyawx_collection/plugins/modules/organization.pyawx_collection/test/awx/test_inventory.pyawx_collection/test/awx/test_job_template.pyawx_collection/test/awx/test_organization.py
🚧 Files skipped from review as they are similar to previous changes (3)
- awx_collection/plugins/modules/job_template.py
- awx_collection/test/awx/test_job_template.py
- awx_collection/test/awx/test_organization.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
The current awx.awx / ansible.controller collections do not support the setting and modifying of the opa_query_path parameter for the following objects:
Note that modifying the opa_query_path for organization must be done using the ansible.platform collection, which will need to be modified separately.
ISSUE TYPE
COMPONENT NAME
ADDITIONAL INFORMATION
The opa_query_path can be obtained for existing inventories, and added for new inventories, from the API at:
/api/controller/v2/inventories/
And similarly for job templates:
/api/controller/v2/job_templates/
Although this PR also updates the awx.awx.organization role, as of AAP 2.5 the organization endpoint should be read-only. Organizations should be created and modified via the platform API (and the ansible.platform collection).
Summary by CodeRabbit
New Features
Documentation
Tests