Conversation
RUN-4228: allow to set ansible base dir project wide
There was a problem hiding this comment.
Pull request overview
This PR extends the Ansible plugin’s configuration resolution so ansible-base-dir-path can be set at the project (and framework) level and be picked up consistently by playbook/module workflow steps, node steps, and the node executor.
Changes:
- Added project/framework property mappings for
ansible-base-dir-pathacross multiple plugin descriptions. - Updated
AnsibleRunnerContextBuilder#getBaseDir()to resolve viaPropertyResolver(job/node/project/framework precedence) instead of job-conf only. - Documented
ansible-base-dir-pathin the README configuration attributes list.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/groovy/com/rundeck/plugins/ansible/plugin/AnsiblePlaybookWorkflowStep.java | Adds project/framework mapping for base dir to the playbook workflow step descriptor. |
| src/main/groovy/com/rundeck/plugins/ansible/plugin/AnsiblePlaybookWorflowNodeStep.java | Adds project/framework mapping for base dir to the playbook workflow node step descriptor. |
| src/main/groovy/com/rundeck/plugins/ansible/plugin/AnsiblePlaybookInlineWorkflowStep.java | Adds project/framework mapping for base dir to the inline playbook workflow step descriptor. |
| src/main/groovy/com/rundeck/plugins/ansible/plugin/AnsiblePlaybookInlineWorkflowNodeStep.java | Adds project/framework mapping for base dir to the inline playbook workflow node step descriptor. |
| src/main/groovy/com/rundeck/plugins/ansible/plugin/AnsibleNodeExecutor.java | Adds project/framework mapping entries for base dir in the node executor descriptor. |
| src/main/groovy/com/rundeck/plugins/ansible/plugin/AnsibleModuleWorkflowStep.java | Adds project/framework mapping for base dir to the module workflow step descriptor. |
| src/main/groovy/com/rundeck/plugins/ansible/ansible/AnsibleRunnerContextBuilder.java | Changes getBaseDir() to resolve base dir via PropertyResolver (enabling project/framework-level config). |
| README.md | Documents ansible-base-dir-path as a configurable attribute. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| builder.mapping(ANSIBLE_CONFIG_FILE_PATH,PROJ_PROP_PREFIX + ANSIBLE_CONFIG_FILE_PATH); | ||
| builder.frameworkMapping(ANSIBLE_CONFIG_FILE_PATH,FWK_PROP_PREFIX + ANSIBLE_CONFIG_FILE_PATH); | ||
| builder.mapping(ANSIBLE_BASE_DIR_PATH,PROJ_PROP_PREFIX + ANSIBLE_BASE_DIR_PATH); | ||
| builder.frameworkMapping(ANSIBLE_BASE_DIR_PATH,FWK_PROP_PREFIX + ANSIBLE_BASE_DIR_PATH); |
There was a problem hiding this comment.
ANSIBLE_BASE_DIR_PATH is being added to the project/framework mappings, but this plugin description does not declare the corresponding BASE_DIR_PROP in its builder.property(...) list. If Rundeck only applies mappings for declared properties (as it appears to do elsewhere in the codebase), this mapping may be ignored and the setting will also not be visible/configurable in the Node Executor UI. Consider adding builder.property(BASE_DIR_PROP) here (or remove the mapping if the Node Executor should not expose base dir at all).
| public String getBaseDir() { | ||
| String baseDir = null; | ||
| if (getJobConf().containsKey(AnsibleDescribable.ANSIBLE_BASE_DIR_PATH)) { | ||
| baseDir = (String) jobConf.get(AnsibleDescribable.ANSIBLE_BASE_DIR_PATH); | ||
| } | ||
| String baseDir; | ||
| baseDir = PropertyResolver.resolveProperty( | ||
| AnsibleDescribable.ANSIBLE_BASE_DIR_PATH, | ||
| null, | ||
| getFrameworkProject(), | ||
| getFramework(), | ||
| getNode(), | ||
| getJobConf() | ||
| ); |
There was a problem hiding this comment.
This change adds project/framework-aware resolution for ansible-base-dir-path via PropertyResolver. There are existing specs for AnsibleRunnerContextBuilder, but none cover getBaseDir() precedence (job vs node vs project vs framework) or variable expansion. Adding a focused unit test for getBaseDir() would help prevent regressions and validate the new project-wide behavior.
|
Hi @fdevans, do you have an estimate of which release this PR might be included in? |
This is a community PR merged locally to run further testing and validate.
Original PR is: #422 from @hatch01