mgmt-23250: Expose AAP configuration from the LZ#31
Conversation
|
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:
WalkthroughAdds AAP archive support: new defaults for archive control, a new task to build/serve a config tarball via Podman, playbook changes to run the archive service before VMaaS, and an archive URI injected into the AAP config template. Changes
Sequence Diagram(s)sequenceDiagram
participant Playbook as Playbook (VMaaS validation)
participant ArchTask as AAP Archive Task
participant FS as Filesystem
participant Git as Git Repository
participant Podman as Podman Container
participant VMaaS as VMaaS Deployment
Playbook->>ArchTask: invoke aap_archive_service
ArchTask->>FS: stat `aap_archive_dest`
alt tarball missing or `aap_archive_refresh` true
ArchTask->>FS: mkdir `aap_archive_tmp_dir`
ArchTask->>Git: git clone `aap_config_repo` @ `aap_config_version`
Git-->>FS: repo checked out
ArchTask->>FS: create gz tarball at `aap_archive_dest`
ArchTask->>FS: remove `aap_archive_tmp_dir` (always)
end
ArchTask->>Podman: run/recreate `aap-archive-srv` (image, auth, volumes, port)
Podman-->>Playbook: archive service ready
Playbook->>VMaaS: proceed with VMaaS deployment
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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)
playbooks/templates/aap-config.yaml.j2 (1)
8-8: Make archive URI scheme configurable for safer transport options.Line 8 hardcodes
http://. Consider templating the scheme so TLS can be enabled without changing template logic.Proposed change
- AAP_PROJECT_ARCHIVE_URI: "http://{{ lzBmcIP }}:{{ aap_archive_port }}/aap-config-archive.tar.gz" + AAP_PROJECT_ARCHIVE_URI: "{{ aap_archive_scheme | default('http') }}://{{ lzBmcIP }}:{{ aap_archive_port }}/aap-config-archive.tar.gz"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playbooks/templates/aap-config.yaml.j2` at line 8, The template hardcodes the URL scheme in AAP_PROJECT_ARCHIVE_URI; make the scheme configurable by replacing the fixed "http://" with a templated variable (e.g., aap_archive_scheme) and defaulting to "http" so callers can set "https" when needed; update the AAP_PROJECT_ARCHIVE_URI assignment to use {{ aap_archive_scheme | default('http') }}://{{ lzBmcIP }}:{{ aap_archive_port }}/aap-config-archive.tar.gz and document the new variable for callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@defaults/vmaas_operators.yaml`:
- Line 18: The aap_config_version currently uses the mutable branch name "main";
update the aap_config_version value to an immutable ref (a specific tag like
"vX.Y.Z" or a commit SHA) so archives don't drift—locate the aap_config_version
entry in defaults/vmaas_operators.yaml and replace "main" with the chosen tag or
SHA and commit the change.
In `@playbooks/tasks/aap_archive_service.yaml`:
- Line 62: The file ends without a terminating newline which breaks YAML/Ansible
validation; open the file containing the mapping line "- \"{{ aap_archive_port
}}:8080\"" and ensure you add a single trailing newline character at the end of
the file (so the last line is terminated), then save and re-run CI.
---
Nitpick comments:
In `@playbooks/templates/aap-config.yaml.j2`:
- Line 8: The template hardcodes the URL scheme in AAP_PROJECT_ARCHIVE_URI; make
the scheme configurable by replacing the fixed "http://" with a templated
variable (e.g., aap_archive_scheme) and defaulting to "http" so callers can set
"https" when needed; update the AAP_PROJECT_ARCHIVE_URI assignment to use {{
aap_archive_scheme | default('http') }}://{{ lzBmcIP }}:{{ aap_archive_port
}}/aap-config-archive.tar.gz and document the new variable for callers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5b668fb6-91c3-439e-bef0-ddce44f1f496
📒 Files selected for processing (5)
defaults/vmaas_operators.yamlplaybooks/06-day2.yamlplaybooks/tasks/aap_archive_service.yamlplaybooks/tasks/vmaas_validation.yamlplaybooks/templates/aap-config.yaml.j2
| aap_license_file: "{{ workingDir }}/config/aap-license.zip" | ||
| aap_archive_tmp_dir: "/tmp/aap-config-source" | ||
| aap_config_repo: "https://github.com/osac-project/osac-aap-config.git" | ||
| aap_config_version: "main" |
There was a problem hiding this comment.
❓ Verification inconclusive
Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd 'vmaas_operators.yaml' | head -n1)"
echo "Inspecting: $file"
rg -n '^\s*aap_config_repo:\s*' "$file"
rg -n '^\s*aap_config_version:\s*' "$file"
python - <<'PY'
import re, pathlib
p = next(pathlib.Path('.').rglob('vmaas_operators.yaml'))
text = p.read_text()
m = re.search(r'^\s*aap_config_version:\s*"?([^"\n]+)"?\s*$', text, re.M)
if not m:
print("aap_config_version not found")
raise SystemExit(1)
v = m.group(1).strip()
is_sha = bool(re.fullmatch(r'[0-9a-f]{7,40}', v))
print(f"aap_config_version={v}")
print("immutable_sha=" + str(is_sha))
PYRepository: rh-ecosystem-edge/enclave
Repository: rh-ecosystem-edge/enclave
Exit code: 0
stdout:
Inspecting: defaults/vmaas_operators.yaml
17:aap_config_repo: "https://github.com/osac-project/osac-aap-config.git"
18:aap_config_version: "main"
aap_config_version=main
immutable_sha=False
Pin aap_config_version to an immutable ref.
Line 18 uses main, which makes archive contents drift over time. For reproducible and auditable deployments, use a tag or commit SHA instead of a branch reference.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@defaults/vmaas_operators.yaml` at line 18, The aap_config_version currently
uses the mutable branch name "main"; update the aap_config_version value to an
immutable ref (a specific tag like "vX.Y.Z" or a commit SHA) so archives don't
drift—locate the aap_config_version entry in defaults/vmaas_operators.yaml and
replace "main" with the chosen tag or SHA and commit the change.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
playbooks/templates/aap-config.yaml.j2 (1)
8-8: HTTP transport for configuration archive.The URI uses plain HTTP. If the archive contains sensitive configuration data (credentials, tokens, etc.), consider whether HTTPS would be more appropriate for your security posture, or document that this endpoint is expected to be on a trusted internal network only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playbooks/templates/aap-config.yaml.j2` at line 8, The AAP_PROJECT_ARCHIVE_URI template currently hardcodes an http scheme; update the template so the transport is explicit and configurable: replace the fixed "http://" with a variable (e.g., {{ aap_archive_scheme }} or a boolean {{ aap_archive_use_https }} that resolves to "https://" when true) and document the expected network posture/default (internal trusted network) in the template or values file; reference the AAP_PROJECT_ARCHIVE_URI key so reviewers can verify the scheme is configurable or switched to "https" for sensitive archives.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@playbooks/templates/aap-config.yaml.j2`:
- Line 8: The AAP_PROJECT_ARCHIVE_URI template uses the user-provided variable
lzBmcIP which can be empty and produce a malformed URI; add validation or a
default to fail fast by either applying a Jinja default/validation filter to
lzBmcIP in the AAP_PROJECT_ARCHIVE_URI line (so the template raises a clear
error or substitutes a safe default) or add an assertion in the calling playbook
to check lzBmcIP is defined/non-empty before rendering; refer to the
AAP_PROJECT_ARCHIVE_URI template and the lzBmcIP variable when implementing the
change.
---
Nitpick comments:
In `@playbooks/templates/aap-config.yaml.j2`:
- Line 8: The AAP_PROJECT_ARCHIVE_URI template currently hardcodes an http
scheme; update the template so the transport is explicit and configurable:
replace the fixed "http://" with a variable (e.g., {{ aap_archive_scheme }} or a
boolean {{ aap_archive_use_https }} that resolves to "https://" when true) and
document the expected network posture/default (internal trusted network) in the
template or values file; reference the AAP_PROJECT_ARCHIVE_URI key so reviewers
can verify the scheme is configurable or switched to "https" for sensitive
archives.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b0643801-1193-427f-ac1d-61c46784afe2
📒 Files selected for processing (5)
defaults/vmaas_operators.yamlplaybooks/06-day2.yamlplaybooks/tasks/aap_archive_service.yamlplaybooks/tasks/vmaas_validation.yamlplaybooks/templates/aap-config.yaml.j2
🚧 Files skipped from review as they are similar to previous changes (2)
- playbooks/tasks/vmaas_validation.yaml
- playbooks/tasks/aap_archive_service.yaml
| namespace: {{ aap_ns | default('ansible-aap') }} | ||
| type: Opaque | ||
| stringData: | ||
| AAP_PROJECT_ARCHIVE_URI: "http://{{ lzBmcIP }}:{{ aap_archive_port }}/aap-config-archive.tar.gz" |
There was a problem hiding this comment.
Add a default filter or validation for lzBmcIP.
Unlike aap_archive_port which has a default in defaults/vmaas_operators.yaml, lzBmcIP is user-provided (per config/global.example.yaml). If the user hasn't configured this variable, the template will render with an empty or undefined value, resulting in a malformed URI like http://:4480/....
Consider adding validation or a default filter to fail fast with a clear error message.
🛡️ Proposed fix to add validation
+{% if lzBmcIP is not defined or lzBmcIP == '' %}
+{{ 'lzBmcIP must be defined in config/global.yaml' | mandatory }}
+{% endif %}
apiVersion: v1
kind: Secret
metadata:Or use an assertion in the calling playbook before rendering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playbooks/templates/aap-config.yaml.j2` at line 8, The
AAP_PROJECT_ARCHIVE_URI template uses the user-provided variable lzBmcIP which
can be empty and produce a malformed URI; add validation or a default to fail
fast by either applying a Jinja default/validation filter to lzBmcIP in the
AAP_PROJECT_ARCHIVE_URI line (so the template raises a clear error or
substitutes a safe default) or add an assertion in the calling playbook to check
lzBmcIP is defined/non-empty before rendering; refer to the
AAP_PROJECT_ARCHIVE_URI template and the lzBmcIP variable when implementing the
change.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
playbooks/tasks/aap_archive_service.yaml (1)
40-43: Jinja2 boolean comparison issue:is falsemay not behave as expected.In Jinja2/Ansible,
is falsetests for the literal booleanFalse, butstat.existsmay be undefined when the stat fails. The condition on line 41 could behave unexpectedly. Additionally, the order of conditions can be optimized - checkis not definedfirst to short-circuit.Consider restructuring for clarity and correctness:
♻️ Proposed fix
when: > - __aap_archive_tarball_exists.stat.exists is false or - __aap_archive_tarball_exists.stat.exists is not defined or + __aap_archive_tarball_exists.stat.exists is not defined or + not (__aap_archive_tarball_exists.stat.exists | bool) or aap_archive_refresh | default(false) | bool🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playbooks/tasks/aap_archive_service.yaml` around lines 40 - 43, The when condition uses "is false" which can mis-handle undefined stat results; change the expression to explicitly handle undefined and false by short-circuiting: replace the current condition with something like "when: not (__aap_archive_tarball_exists.stat.exists | default(false)) or (aap_archive_refresh | default(false) | bool)" so you check "is not defined/false" via the default(false) or a direct not, and keep aap_archive_refresh evaluation with its existing default and |bool conversion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@playbooks/tasks/aap_archive_service.yaml`:
- Around line 40-43: The when condition uses "is false" which can mis-handle
undefined stat results; change the expression to explicitly handle undefined and
false by short-circuiting: replace the current condition with something like
"when: not (__aap_archive_tarball_exists.stat.exists | default(false)) or
(aap_archive_refresh | default(false) | bool)" so you check "is not
defined/false" via the default(false) or a direct not, and keep
aap_archive_refresh evaluation with its existing default and |bool conversion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8688e368-8959-4bd0-a209-c7f97844835c
📒 Files selected for processing (5)
defaults/vmaas_operators.yamlplaybooks/06-day2.yamlplaybooks/tasks/aap_archive_service.yamlplaybooks/tasks/vmaas_validation.yamlplaybooks/templates/aap-config.yaml.j2
🚧 Files skipped from review as they are similar to previous changes (2)
- defaults/vmaas_operators.yaml
- playbooks/templates/aap-config.yaml.j2
Signed-off-by: Manuel Lorenzo <mlorenzofr@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@playbooks/tasks/aap_archive_service.yaml`:
- Around line 18-28: The archive task ("Create tarball of the repository") is
currently packing the whole clone ({{ aap_archive_tmp_dir }}) including .git
metadata; update the ansible.builtin.archive invocation (task name "Create
tarball of the repository") to exclude the repo metadata by adding exclude_path:
"{{ aap_archive_tmp_dir }}/.git" so the produced {{ aap_archive_dest }} tarball
omits .git, or if you need pattern matching for multiple hidden dirs swap to
community.general.archive and set exclusion_patterns like
["**/.git","**/.git/**"] on that task.
In `@playbooks/tasks/vmaas_validation.yaml`:
- Around line 37-41: The include task named "Deploy AAP configuration archive
service in the LZ" currently applies only the aap_archive_service tag to the
included file; update the include's apply.tags to propagate the vmaas tag as
well (so tasks inside aap_archive_service.yaml inherit both aap_archive_service
and vmaas), e.g., change the apply tags for the include of
aap_archive_service.yaml to include both 'aap_archive_service' and 'vmaas'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 671e236e-1eb6-4a9c-9096-98d43e0a47d1
📒 Files selected for processing (5)
defaults/vmaas_operators.yamlplaybooks/06-day2.yamlplaybooks/tasks/aap_archive_service.yamlplaybooks/tasks/vmaas_validation.yamlplaybooks/templates/aap-config.yaml.j2
🚧 Files skipped from review as they are similar to previous changes (3)
- playbooks/06-day2.yaml
- playbooks/templates/aap-config.yaml.j2
- defaults/vmaas_operators.yaml
| - name: "Clone the configuration repository" | ||
| ansible.builtin.git: | ||
| repo: "{{ aap_config_repo }}" | ||
| dest: "{{ aap_archive_tmp_dir }}" | ||
| version: "{{ aap_config_version }}" | ||
|
|
||
| - name: "Create tarball of the repository" | ||
| ansible.builtin.archive: | ||
| path: "{{ aap_archive_tmp_dir }}" | ||
| dest: "{{ aap_archive_dest }}" | ||
| format: gz |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
ansible.builtin.archive module exclude .git hidden directories documentation
💡 Result:
To exclude .git (and other “hidden” dot-directories) when creating an archive, use community.general.archive and prefer exclusion_patterns when the dot-directories are inside the paths you’re archiving.
exclude_pathonly excludes items from the top-levelpathlist and its glob expansion (it expects remote absolute paths or globs). If you’re archiving/repoand want to skip/repo/.git,exclude_pathwon’t help unless.gitis part of what yourpath/glob expands to. [1]exclusion_patternsexcludes files/dirs below any archived path using glob-style patterns—this is the right tool to drop.git/anywhere in the tree. [1]
Example (exclude .git and any dot-directories anywhere under /repo):
- name: Archive repo without VCS + hidden dirs
community.general.archive:
path: /repo
dest: /tmp/repo.tar.gz
format: gz
exclusion_patterns:
- "**/.git"
- "**/.git/**"
- "**/.*"
- "**/.*/*"Source: exclude_path vs exclusion_patterns parameter behavior and definitions. [1]
References
[1] Ansible Community Docs – community.general.archive module (parameters exclude_path, exclusion_patterns). (docs.ansible.com)
Citations:
- 1: https://docs.ansible.com/projects/ansible/latest/collections/community/general/archive_module.html
🌐 Web query:
ansible.builtin.archive module parameters documentation exclude
💡 Result:
In the archive module, there isn’t a parameter literally named exclude. To exclude content, use:
exclude_path: Remote absolute path, glob, or list of paths/globs to exclude from what you specified inpath. This is the exclusion mechanism documented for the (older) built-inarchivemodule. [1]- If you’re actually using
community.general.archive(often used in modern Ansible installs), you also have:exclude_path: excludes specific source paths (afterpathlist/glob expansion). [2]exclusion_patterns: glob-style patterns to exclude files/dirs below any includedpath(different behavior thanexclude_path). [2]
Example (exclude specific directories):
- community.general.archive:
path:
- /path/to/foo/*
dest: /path/file.tar.bz2
exclude_path:
- /path/to/foo/bar
- /path/to/foo/baz[1][2]
Sources:
[1] Ansible archive module docs (archive_module) – exclude_path definition and examples
[2] community.general.archive module docs – exclude_path vs exclusion_patterns
Exclude .git metadata from the published archive.
Line 25 archives the entire clone directory created at lines 18–23, which includes .git/ and VCS metadata. This bloats the payload and exposes repository history/configuration over HTTP. Add exclude_path to skip it:
Suggested fix
- name: "Create tarball of the repository"
ansible.builtin.archive:
path: "{{ aap_archive_tmp_dir }}"
dest: "{{ aap_archive_dest }}"
format: gz
exclude_path: "{{ aap_archive_tmp_dir }}/.git"Alternatively, switch to community.general.archive with exclusion_patterns for more robust pattern matching (e.g., ["**/.git", "**/.git/**"]) if filtering multiple hidden directories.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playbooks/tasks/aap_archive_service.yaml` around lines 18 - 28, The archive
task ("Create tarball of the repository") is currently packing the whole clone
({{ aap_archive_tmp_dir }}) including .git metadata; update the
ansible.builtin.archive invocation (task name "Create tarball of the
repository") to exclude the repo metadata by adding exclude_path: "{{
aap_archive_tmp_dir }}/.git" so the produced {{ aap_archive_dest }} tarball
omits .git, or if you need pattern matching for multiple hidden dirs swap to
community.general.archive and set exclusion_patterns like
["**/.git","**/.git/**"] on that task.
| - name: "Deploy AAP configuration archive service in the LZ" | ||
| ansible.builtin.include_tasks: | ||
| file: aap_archive_service.yaml | ||
| apply: | ||
| tags: aap_archive_service |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the relevant files mentioned in the review
git ls-files | grep -E "(vmaas_validation|06-day2)" | head -20Repository: rh-ecosystem-edge/enclave
Length of output: 129
🏁 Script executed:
# Also search for these files more broadly
find . -name "vmaas_validation.yaml" -o -name "06-day2.yaml" 2>/dev/nullRepository: rh-ecosystem-edge/enclave
Length of output: 133
🌐 Web query:
Ansible include_tasks apply tags filtering behavior with --tags flag
💡 Result:
With ansible.builtin.include_tasks (a dynamic include), tag filtering with ansible-playbook --tags ... is effectively two-stage:
-
The
include_tasksstatement is itself a task, so it must match--tags(or betags: always) to even be executed (and thus to “descend into” the included file). Tags set on the include line apply only to the include task, not automatically to the tasks inside. [1] -
After the file is included, tasks inside are still filtered by
--tags. If those inner tasks don’t have a matching tag, they’ll be skipped—even though the include ran. [1]
How to make --tags X run all tasks from the included file
Use apply: to propagate keywords (including tags) onto every task inside the included file:
- name: Include my tasks
ansible.builtin.include_tasks: mytasks.yml
tags: [mytag] # ensures the include itself runs when using --tags mytag
apply:
tags: [mytag] # tags every task inside mytasks.yml with mytagapply is specifically documented as “task keywords (for example tags) that will be applied to the tasks within the include.” [2]
If you want tag inheritance without apply
Prefer the static form import_tasks, where tag inheritance behaves the way people often expect (tags applied to the import apply to tasks in the imported file). Dynamic includes like include_tasks are called out as not using tag inheritance by default. [1]
Sources:
[1] Ansible tags guide (tag inheritance vs dynamic includes, and --tags behavior with dynamic includes)
[2] ansible.builtin.include_tasks module docs (apply: keyword)
🏁 Script executed:
# Read vmaas_validation.yaml around lines 37-41
sed -n '30,50p' ./playbooks/tasks/vmaas_validation.yamlRepository: rh-ecosystem-edge/enclave
Length of output: 684
🏁 Script executed:
# Read 06-day2.yaml around lines 85-96 to see how this file is invoked
sed -n '80,100p' ./playbooks/06-day2.yamlRepository: rh-ecosystem-edge/enclave
Length of output: 562
Propagate the vmaas tag into the archive-service include.
Line 41 only applies aap_archive_service to tasks inside aap_archive_service.yaml. When playbooks/06-day2.yaml invokes this file with both tags, --tags vmaas will reach the include task (which inherits the vmaas tag from the parent's apply directive) but skip every child task, so the new prerequisite service never starts in the tagged VMaaS flow.
Suggested change
- name: "Deploy AAP configuration archive service in the LZ"
ansible.builtin.include_tasks:
file: aap_archive_service.yaml
apply:
- tags: aap_archive_service
+ tags:
+ - vmaas
+ - aap_archive_service🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playbooks/tasks/vmaas_validation.yaml` around lines 37 - 41, The include task
named "Deploy AAP configuration archive service in the LZ" currently applies
only the aap_archive_service tag to the included file; update the include's
apply.tags to propagate the vmaas tag as well (so tasks inside
aap_archive_service.yaml inherit both aap_archive_service and vmaas), e.g.,
change the apply tags for the include of aap_archive_service.yaml to include
both 'aap_archive_service' and 'vmaas'.
3 participants
Summary by CodeRabbit
New Features
Configuration