AAP-68210 Use FQCN throughout collection, and reduce dynamic templating in favor of release-based#16347
AAP-68210 Use FQCN throughout collection, and reduce dynamic templating in favor of release-based#16347AlanCoding wants to merge 6 commits intoansible:develfrom
Conversation
📝 WalkthroughWalkthroughThis change introduces a flexible collection identity system that allows the AWX collection to be distributed with different FQCNs depending on the repository. It establishes environment variables for namespace and package, implements dynamic FQCN derivation in core utilities, updates module documentation to use fully-qualified names, and adds infrastructure for converting FQCN references during release branches. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~90 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 |
github-actions
bot
added
the
component:awx_collection
AlanCoding
force-pushed
the
collection_fqcn
branch
from
443b747 to
b89e6f1
Compare
The jinja2 templates for README and galaxy.yml are removed In favor of this, a script is added with simplified goals the script has the objective of: - bulk replacing awx.awx with ansible.controller in source - replacing special cases, mainly putting in version A corresponding github actions test is added this check will fail on tower PRs if the replacement has not been ran The script has not been ran, and will be done in later commits
AlanCoding
force-pushed
the
collection_fqcn
branch
from
b89e6f1 to
2fc1eca
Compare
|
AlanCoding
changed the title
There was a problem hiding this comment.
I have strongly considered having the script delete itself after running. Just to put a finer point on the thought process here.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| def api_path(self, app_key=None): | ||
|
|
||
| if self.api_prefix: | ||
| return self.api_prefix |
There was a problem hiding this comment.
api_prefix bypasses URL path normalization in api_path
Medium Severity
When self.api_prefix is set, api_path() returns it immediately, bypassing the normalization that ensures the path starts with / and ends with / (lines 664–668). A value like "api" or "api/controller" from a config file would produce a malformed URL in build_url(), while the same value set via CONTROLLER_OPTIONAL_API_URLPATTERN_PREFIX env var through the old code path would be properly normalized.
There was a problem hiding this comment.
I would rather not mess with it and let the user figure it out via the value of CONTROLLER_OPTIONAL_API_URLPATTERN_PREFIX. The more we try to automatically fix things, the more we own those problems.
| # On upstream (awx) this is "awx.awx"; on downstream (tower) the | ||
| # replace_fqcn.sh script converts it to "ansible.controller". | ||
| # _COLLECTION_TYPE is derived from the FQCN automatically. | ||
| _COLLECTION_FQCN = "awx.awx" |
There was a problem hiding this comment.
This syntax change allows this line to get swept up in the general find+replace. It's dumb logic that takes all "awx.awx" and turns it into "ansible.controller".
This still leaves a special case for the version when packaging a collection.
So the collection FQCN is bulk-replaced by the script and it must go in a commit. Then the version is inserted in before building, every time.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
awx_collection/plugins/modules/schedule.py (1)
177-177:⚠️ Potential issue | 🟡 MinorMissing quotes around plugin name in query.
The plugin name
awx.awx.schedule_rrulesetshould be quoted as a string, similar to line 169. Without quotes, Jinja2 will attempt variable attribute access instead of treating it as a plugin reference.🐛 Proposed fix
- rrule: "{{ query(awx.awx.schedule_rruleset, '2022-04-30 10:30:45', rules=rrules, timezone='UTC' ) | first }}" + rrule: "{{ query('awx.awx.schedule_rruleset', '2022-04-30 10:30:45', rules=rrules, timezone='UTC' ) | first }}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx_collection/plugins/modules/schedule.py` at line 177, The query call passing the plugin name awx.awx.schedule_rruleset is missing quotes and thus will be interpreted as attribute access; update the template expression to quote the plugin name (e.g., use "awx.awx.schedule_rruleset") in the rrule assignment so Jinja2 treats it as a string plugin reference (match the quoted usage on the earlier line around the existing query call).awx_collection/plugins/modules/subscriptions.py (1)
62-75:⚠️ Potential issue | 🟠 MajorReplace the example
client_secretwith an obvious placeholder.These docs now publish a secret-looking token twice, and secret scanning is already flagging Line 66. Even if it is fake, keeping realistic secrets in examples creates avoidable leakage risk and CI noise.
Suggested doc-only fix
EXAMPLES = ''' - name: Get subscriptions awx.awx.subscriptions: client_id: "c6bd7594-d776-46e5-8156-6d17af147479" - client_secret: "MO9QUvoOZ5fc5JQKXoTch1AsTLI7nFsZ" + client_secret: "YOUR_CLIENT_SECRET" - name: Get subscriptions with a filter awx.awx.subscriptions: client_id: "c6bd7594-d776-46e5-8156-6d17af147479" - client_secret: "MO9QUvoOZ5fc5JQKXoTch1AsTLI7nFsZ" + client_secret: "YOUR_CLIENT_SECRET" filters: product_name: "Red Hat Ansible Automation Platform" support_level: "Self-Support" '''🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx_collection/plugins/modules/subscriptions.py` around lines 62 - 75, Update the EXAMPLES string in subscriptions.py to remove the realistic-looking client_secret values and replace them with an obvious placeholder (e.g. REPLACE_WITH_CLIENT_SECRET or <CLIENT_SECRET>) so the example under the EXAMPLES constant (which includes the client_id/client_secret pairs) no longer contains secret-like tokens; ensure both occurrences of client_secret in the multi-example block are updated to the placeholder.
🧹 Nitpick comments (4)
awx_collection/test/awx/conftest.py (1)
192-195: Scope the/api/override more narrowly.This shared fixture now forces every
run_moduletest to bypass the default API-prefix resolution incontroller_api.py/awxkit.py. That means regressions in the new FQCN-driven upstream/downstream path selection can still pass here. Prefer an opt-in fixture for tests that specifically need the standalone AWX server to stay on/api/.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
.github/workflows/ci.yml (1)
250-256: Consider reusingreplace_fqcn.shinstead of duplicating sed logic.This sed command duplicates the logic in
awx_collection/tools/replace_fqcn.sh. If the replacement pattern needs to change (e.g., adding more file types or adjusting the pattern), both locations would need updating.Consider invoking the script or extracting the pattern to avoid drift:
♻️ Potential refactor
- name: Convert awx-operator playbooks to ansible.controller FQCN if: github.repository == 'ansible/tower' working-directory: awx-operator run: | - find molecule/ -type f \( -name '*.yml' -o -name '*.yaml' \) \ - -exec sed -i 's/awx\.awx/ansible.controller/g' {} + + # Reuse the same replacement pattern as replace_fqcn.sh + ../awx/awx_collection/tools/replace_fqcn.sh molecule/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 250 - 256, The CI step currently runs an inline sed that duplicates the awx_collection/tools/replace_fqcn.sh logic; replace that sed invocation with a call to the existing replace_fqcn.sh script (or refactor both locations to source a shared pattern) so the FQCN replacement is centralized—update the workflow step that runs the sed command to execute replace_fqcn.sh (passing the target directory, e.g., molecule/) or call a small wrapper that forwards arguments to replace_fqcn.sh and ensure the script handles the same file globs and replacement pattern used by the current sed.awx_collection/tools/replace_fqcn.sh (1)
23-24: Portability:sed -iwithout suffix fails on macOS.The
sed -iflag requires an empty suffix argument on BSD/macOS (sed -i '' ...). If this script is only intended to run in CI (Linux), this is acceptable. Otherwise, consider using a portable pattern:♻️ Portable sed alternative
- sed -i 's/^namespace: awx$/namespace: ansible/' "$COLLECTION_ROOT/galaxy.yml" - sed -i 's/^name: awx$/name: controller/' "$COLLECTION_ROOT/galaxy.yml" + sed -i.bak 's/^namespace: awx$/namespace: ansible/' "$COLLECTION_ROOT/galaxy.yml" && rm -f "$COLLECTION_ROOT/galaxy.yml.bak" + sed -i.bak 's/^name: awx$/name: controller/' "$COLLECTION_ROOT/galaxy.yml" && rm -f "$COLLECTION_ROOT/galaxy.yml.bak"Or use a function:
portable_sed_i() { if [[ "$OSTYPE" == "darwin"* ]]; then sed -i '' "$@" else sed -i "$@" fi }Also applies to: 30-31
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx_collection/tools/replace_fqcn.sh` around lines 23 - 24, The sed -i usage in replace_fqcn.sh is not portable to macOS; replace the direct sed -i calls (the two lines that run sed on "$COLLECTION_ROOT/galaxy.yml" and the other two at 30-31) with a portable wrapper: add a portable_sed_i function that detects darwin via OSTYPE and invokes sed -i '' on macOS and sed -i on other platforms, then call portable_sed_i in place of the current sed -i invocations (referencing the existing sed command occurrences in replace_fqcn.sh).awx_collection/test/awx/test_module_utils.py (1)
74-76: Verifyassert_any_callis intentional overassert_called_once_with.The change from
assert_called_once_withtoassert_any_callis less strict—it only verifies the warning was called with these arguments at least once, rather than exactly once. If the test should ensure no spurious warnings, consider keeping the stricter assertion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx_collection/test/awx/test_module_utils.py` around lines 74 - 76, The test relaxed the check by using silence_warning.assert_any_call(...) instead of the stricter silence_warning.assert_called_once_with(...); decide if the test must ensure the warning is emitted exactly once and, if so, replace assert_any_call with silence_warning.assert_called_once_with('You are running collection version {0} but connecting to {1} version {2}'.format(my_module._COLLECTION_VERSION, awx_name, ping_version)); if the intent is to allow multiple calls but still ensure at least one occurrence, leave assert_any_call and add an explicit assertEqual(silence_warning.call_count, 1) or a more precise check to enforce the expected call count. Ensure you reference the silence_warning mock and the formatted message in the assertion change.
🤖 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/module_utils/controller_api.py`:
- Around line 644-646: The method currently returns self.api_prefix verbatim,
causing inconsistency with default_api_path normalization; update the logic so
when self.api_prefix is set it is normalized to include a single trailing slash
(e.g., ensure it ends with "/" similar to how other paths are normalized) before
returning, preserving existing behavior when already correct; reference the
symbols self.api_prefix and default_api_path and make the change where the
api_prefix return occurs so endpoint matching code that relies on a trailing
slash receives a normalized value.
In `@awx_collection/README.md`:
- Around line 47-53: The fenced code block showing the YAML-like example (the
block containing "host: https://localhost:8043", "verify_ssl: true",
"oauth_token: <token>") should include a language specifier; update the opening
fence from ``` to ```yaml so the block becomes ```yaml ... ``` to match other
examples and enable proper syntax highlighting.
---
Outside diff comments:
In `@awx_collection/plugins/modules/schedule.py`:
- Line 177: The query call passing the plugin name awx.awx.schedule_rruleset is
missing quotes and thus will be interpreted as attribute access; update the
template expression to quote the plugin name (e.g., use
"awx.awx.schedule_rruleset") in the rrule assignment so Jinja2 treats it as a
string plugin reference (match the quoted usage on the earlier line around the
existing query call).
In `@awx_collection/plugins/modules/subscriptions.py`:
- Around line 62-75: Update the EXAMPLES string in subscriptions.py to remove
the realistic-looking client_secret values and replace them with an obvious
placeholder (e.g. REPLACE_WITH_CLIENT_SECRET or <CLIENT_SECRET>) so the example
under the EXAMPLES constant (which includes the client_id/client_secret pairs)
no longer contains secret-like tokens; ensure both occurrences of client_secret
in the multi-example block are updated to the placeholder.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 250-256: The CI step currently runs an inline sed that duplicates
the awx_collection/tools/replace_fqcn.sh logic; replace that sed invocation with
a call to the existing replace_fqcn.sh script (or refactor both locations to
source a shared pattern) so the FQCN replacement is centralized—update the
workflow step that runs the sed command to execute replace_fqcn.sh (passing the
target directory, e.g., molecule/) or call a small wrapper that forwards
arguments to replace_fqcn.sh and ensure the script handles the same file globs
and replacement pattern used by the current sed.
In `@awx_collection/test/awx/test_module_utils.py`:
- Around line 74-76: The test relaxed the check by using
silence_warning.assert_any_call(...) instead of the stricter
silence_warning.assert_called_once_with(...); decide if the test must ensure the
warning is emitted exactly once and, if so, replace assert_any_call with
silence_warning.assert_called_once_with('You are running collection version {0}
but connecting to {1} version {2}'.format(my_module._COLLECTION_VERSION,
awx_name, ping_version)); if the intent is to allow multiple calls but still
ensure at least one occurrence, leave assert_any_call and add an explicit
assertEqual(silence_warning.call_count, 1) or a more precise check to enforce
the expected call count. Ensure you reference the silence_warning mock and the
formatted message in the assertion change.
In `@awx_collection/tools/replace_fqcn.sh`:
- Around line 23-24: The sed -i usage in replace_fqcn.sh is not portable to
macOS; replace the direct sed -i calls (the two lines that run sed on
"$COLLECTION_ROOT/galaxy.yml" and the other two at 30-31) with a portable
wrapper: add a portable_sed_i function that detects darwin via OSTYPE and
invokes sed -i '' on macOS and sed -i on other platforms, then call
portable_sed_i in place of the current sed -i invocations (referencing the
existing sed command occurrences in replace_fqcn.sh).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71a229dc-88dd-4e54-806e-46dc6e0c5e81
📒 Files selected for processing (63)
.github/workflows/ci.yml.github/workflows/promote.ymlMakefileawx_collection/README.mdawx_collection/galaxy.ymlawx_collection/plugins/doc_fragments/auth_plugin.pyawx_collection/plugins/lookup/controller_api.pyawx_collection/plugins/module_utils/controller_api.pyawx_collection/plugins/modules/ad_hoc_command.pyawx_collection/plugins/modules/ad_hoc_command_cancel.pyawx_collection/plugins/modules/ad_hoc_command_wait.pyawx_collection/plugins/modules/bulk_host_create.pyawx_collection/plugins/modules/bulk_host_delete.pyawx_collection/plugins/modules/bulk_job_launch.pyawx_collection/plugins/modules/controller_meta.pyawx_collection/plugins/modules/credential.pyawx_collection/plugins/modules/credential_input_source.pyawx_collection/plugins/modules/credential_type.pyawx_collection/plugins/modules/execution_environment.pyawx_collection/plugins/modules/export.pyawx_collection/plugins/modules/group.pyawx_collection/plugins/modules/host.pyawx_collection/plugins/modules/import.pyawx_collection/plugins/modules/inventory.pyawx_collection/plugins/modules/inventory_source.pyawx_collection/plugins/modules/inventory_source_update.pyawx_collection/plugins/modules/job_cancel.pyawx_collection/plugins/modules/job_launch.pyawx_collection/plugins/modules/job_list.pyawx_collection/plugins/modules/job_template.pyawx_collection/plugins/modules/job_wait.pyawx_collection/plugins/modules/label.pyawx_collection/plugins/modules/license.pyawx_collection/plugins/modules/notification_template.pyawx_collection/plugins/modules/organization.pyawx_collection/plugins/modules/project.pyawx_collection/plugins/modules/project_update.pyawx_collection/plugins/modules/role.pyawx_collection/plugins/modules/role_definition.pyawx_collection/plugins/modules/role_team_assignment.pyawx_collection/plugins/modules/role_user_assignment.pyawx_collection/plugins/modules/schedule.pyawx_collection/plugins/modules/settings.pyawx_collection/plugins/modules/subscriptions.pyawx_collection/plugins/modules/team.pyawx_collection/plugins/modules/user.pyawx_collection/plugins/modules/workflow_approval.pyawx_collection/plugins/modules/workflow_job_template.pyawx_collection/plugins/modules/workflow_job_template_node.pyawx_collection/plugins/modules/workflow_launch.pyawx_collection/plugins/modules/workflow_node_wait.pyawx_collection/test/awx/conftest.pyawx_collection/test/awx/test_build_url.pyawx_collection/test/awx/test_module_utils.pyawx_collection/tests/sanity/ignore-2.14.txtawx_collection/tests/sanity/ignore-2.15.txtawx_collection/tests/sanity/ignore-2.16.txtawx_collection/tests/sanity/ignore-2.17.txtawx_collection/tools/replace_fqcn.shawx_collection/tools/roles/template_galaxy/tasks/main.ymlawx_collection/tools/roles/template_galaxy/templates/README.md.j2awx_collection/tools/roles/template_galaxy/templates/galaxy.yml.j2awx_collection/tools/template_galaxy.yml
💤 Files with no reviewable changes (3)
- awx_collection/tools/roles/template_galaxy/templates/README.md.j2
- awx_collection/galaxy.yml
- awx_collection/tools/roles/template_galaxy/templates/galaxy.yml.j2
| if self.api_prefix: | ||
| return self.api_prefix | ||
| default_api_path = "/api/" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how api_prefix is used in tests and whether normalization is expected
rg -n "api_prefix" awx_collection/ -A3 -B3Repository: ansible/awx
Length of output: 2622
🏁 Script executed:
#!/bin/bash
# Get the complete api_path method to see the trailing slash normalization
sed -n '642,680p' awx_collection/plugins/module_utils/controller_api.py | cat -nRepository: ansible/awx
Length of output: 1857
🏁 Script executed:
#!/bin/bash
# Also search for how api_path and api_prefix are used in the codebase
rg -n "\.api_path\(|api_prefix" awx_collection/ --type py -A2 -B1 | head -100Repository: ansible/awx
Length of output: 2038
🏁 Script executed:
#!/bin/bash
# Search for test cases and usage of api_prefix to understand expected format
rg -n "api_prefix\s*=" awx_collection/ --type py -A2 -B2 | head -60Repository: ansible/awx
Length of output: 473
🏁 Script executed:
#!/bin/bash
# Check if there are any tests that demonstrate the issue or expected behavior
find awx_collection -name "*test*.py" -type f -exec grep -l "api_prefix" {} \;Repository: ansible/awx
Length of output: 37
Add trailing slash normalization to api_prefix for consistency.
When api_prefix is set, the method returns it directly without the leading/trailing slash normalization applied to other paths (lines 23-27). If a caller passes api_prefix without a trailing slash (e.g., "/api" instead of "/api/"), URL construction will be inconsistent and may break at line 170 where endpoint matching occurs.
Either normalize api_prefix like the default paths, or document and validate that api_prefix must include trailing slashes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@awx_collection/plugins/module_utils/controller_api.py` around lines 644 -
646, The method currently returns self.api_prefix verbatim, causing
inconsistency with default_api_path normalization; update the logic so when
self.api_prefix is set it is normalized to include a single trailing slash
(e.g., ensure it ends with "/" similar to how other paths are normalized) before
returning, preserving existing behavior when already correct; reference the
symbols self.api_prefix and default_api_path and make the change where the
api_prefix return occurs so endpoint matching code that relies on a trailing
slash receives a normalized value.
There was a problem hiding this comment.
@AlanCoding, understood — no worries, acknowledged!
| or like this: | ||
|
|
||
| ``` | ||
| host: https://localhost:8043 | ||
| verify_ssl: true | ||
| oauth_token: <token> | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to fenced code block.
The code block on line 49 is missing a language specifier. For consistency with other code blocks in this file (which use no language but contain shell/ini), consider adding yaml since this example shows YAML format.
📝 Suggested fix
or like this:
-```
+```yaml
host: https://localhost:8043
verify_ssl: true
oauth_token: <token>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| or like this: | |
| ``` | |
| host: https://localhost:8043 | |
| verify_ssl: true | |
| oauth_token: <token> | |
| ``` | |
| or like this: | |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 49-49: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@awx_collection/README.md` around lines 47 - 53, The fenced code block showing
the YAML-like example (the block containing "host: https://localhost:8043",
"verify_ssl: true", "oauth_token: <token>") should include a language specifier;
update the opening fence from ``` to ```yaml so the block becomes ```yaml ...
``` to match other examples and enable proper syntax highlighting.
There was a problem hiding this comment.
This is templated from the prior README.md.j2
This is not in the scope of the PR. All this does it materialize content already in the template to the duplicated file. This is the content already being used when building the collection.
I didn't start this to fix this problem, and it's noise.
There was a problem hiding this comment.
@AlanCoding, understood — this is pre-existing content from the template that the PR simply materializes, and fixing the missing language specifier is out of scope here. I'll note this for future reference.
📝 Learnings were identified and not saved (knowledge base disabled). Enable
3 participants
SUMMARY
Replacement for #16343
This puts FQCN everywhere like
awx.awx.host, and allows a what-you-see-is-what-you-get.As a tradeoff, this introduces a downstream-ify script that must be ran for every major release branch to transform into the
ansible.controllercollection, or else things will break.ISSUE TYPE
COMPONENT NAME
Note
Medium Risk
Touches broad collection-facing surfaces (docs/examples, build/release automation, CI, and API URL construction), so a missed conversion or prefix change could break downstream releases or CI/integration tests.
Overview
Makes the collection “what you see is what you get” by standardizing on explicit FQCNs and shifting downstream renames to a release-branch conversion step. Module/lookup docs and examples are updated to use
awx.awx.*everywhere, whileControllerAPIModulenow derives collection type from a canonical_COLLECTION_FQCNandcontroller_metareports the full FQCN.Build/release plumbing is simplified:
Makefilereadsnamespace/namefromawx_collection/galaxy.yml, thetemplate_galaxyrole now only stamps versions (no more dynamic FQCN/README/galaxy templating), and a newawx_collection/tools/replace_fqcn.shconverts the source tree toansible.controllerfor downstream release branches (with CI checks to ensure conversion happened).CI/promote workflows are updated to parameterize collection namespace/name, adjust artifact paths accordingly, and force
/api/routing for tests via a newapi_prefixoption plusCONTROLLER_OPTIONAL_API_URLPATTERN_PREFIXusage (also applied in awx-operator molecule runs and collection integration tests).Written by Cursor Bugbot for commit b235b84. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Release Notes
New Features
Documentation