httpboot feature for proxy#518
Conversation
| foreman_proxy_bmc_ipmi_implementation: ipmitool | ||
| foreman_proxy_bmc_redfish_verify_ssl: true | ||
|
|
||
| # HTTPBoot settings |
There was a problem hiding this comment.
Seems redundant given the variable name.
|
I am testing out some review logic through AI, and how the output comes out: Task Logic
Tests
|
Yes, thats a concern, the issue was that without tftp feature(which creates the
As of today, by default httpboot enablement will add
thats on my radar, to add such tests, i show we expose couple of setting(https://github.com/theforeman/smart-proxy/blob/develop/modules/httpboot/httpboot_plugin.rb#L15-L16) but not capabilities, so for http support i started #520 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis PR adds HTTPBoot feature support to the foreman_proxy role. The feature is declared in the feature registry and maps to the Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
🧹 Nitpick comments (1)
tests/foreman_proxy_test.py (1)
32-35: ⚡ Quick winAdd a dedicated HTTPBoot
/v2/featuressettings assertion.This only checks feature listing. Please add a feature-scoped test (like BMC tests) to validate
proxy_v2_features['httpboot']['settings']['root_dir']matchesforeman_proxy_httpboot_root_dir, so config/render/mount regressions are caught.As per coding guidelines `tests/*_test.py` should use testinfra's server fixture for infrastructure testing.Example test addition
+@pytest.mark.feature('httpboot') +def test_httpboot_settings(proxy_v2_features): + assert 'httpboot' in proxy_v2_features + settings = proxy_v2_features['httpboot'].get('settings', {}) + assert settings.get('root_dir') == '/var/lib/tftpboot'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/foreman_proxy_test.py` around lines 32 - 35, Add a dedicated assertion that validates the HTTPBoot settings root_dir using the testinfra server fixture: when 'httpboot' is in enabled_features, assert that proxy_v2_features['httpboot']['settings']['root_dir'] equals foreman_proxy_httpboot_root_dir (and keep the existing presence check of "httpboot" in features); when not enabled, assert that 'httpboot' is not present in proxy_v2_features. Place this in tests/foreman_proxy_test.py alongside the existing enabled_features/features checks and use the testinfra server fixture to access proxy_v2_features/foreman_proxy_httpboot_root_dir so config/render/mount regressions are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/roles/foreman_proxy/tasks/feature/httpboot.yaml`:
- Around line 7-17: Add a deterministic guard before the "Mount httpboot root
directory" task: insert a new task named like "Fail if httpboot root dir
missing" that uses ansible.builtin.fail with a clear message and triggers when
not _foreman_proxy_httpboot_root_dir.stat.exists; keep the existing mount task
unchanged (it can remain conditional on
_foreman_proxy_httpboot_root_dir.stat.exists) so the playbook will fail fast
instead of allowing the httpboot template (settings.d/httpboot.yml.j2) to
advertise a non-existent foreman_proxy_httpboot_root_dir.
---
Nitpick comments:
In `@tests/foreman_proxy_test.py`:
- Around line 32-35: Add a dedicated assertion that validates the HTTPBoot
settings root_dir using the testinfra server fixture: when 'httpboot' is in
enabled_features, assert that
proxy_v2_features['httpboot']['settings']['root_dir'] equals
foreman_proxy_httpboot_root_dir (and keep the existing presence check of
"httpboot" in features); when not enabled, assert that 'httpboot' is not present
in proxy_v2_features. Place this in tests/foreman_proxy_test.py alongside the
existing enabled_features/features checks and use the testinfra server fixture
to access proxy_v2_features/foreman_proxy_httpboot_root_dir so
config/render/mount regressions are caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a2c9788c-4e60-44e7-aafd-490222f894fc
📒 Files selected for processing (5)
src/features.yamlsrc/roles/foreman_proxy/defaults/main.yamlsrc/roles/foreman_proxy/tasks/feature/httpboot.yamlsrc/roles/foreman_proxy/templates/settings.d/httpboot.yml.j2tests/foreman_proxy_test.py
| - name: Mount httpboot root directory | ||
| ansible.builtin.copy: | ||
| dest: /etc/containers/systemd/foreman-proxy.container.d/httpboot-volume.conf | ||
| content: | | ||
| [Container] | ||
| Volume={{ foreman_proxy_httpboot_root_dir }}:{{ foreman_proxy_httpboot_root_dir }}:z | ||
| mode: '0644' | ||
| owner: root | ||
| group: root | ||
| when: _foreman_proxy_httpboot_root_dir.stat.exists | ||
| notify: |
There was a problem hiding this comment.
Avoid enabling HTTPBoot with an unmounted root_dir.
When the directory is absent, this task skips the mount but the feature config still advertises :root_dir (from src/roles/foreman_proxy/templates/settings.d/httpboot.yml.j2), so HTTPBoot can be enabled in a broken state. Make this deterministic: either create the directory before mounting or fail explicitly if it does not exist.
Suggested fix (fail fast)
- name: Check if httpboot root directory exists
ansible.builtin.stat:
path: "{{ foreman_proxy_httpboot_root_dir }}"
register: _foreman_proxy_httpboot_root_dir
+- name: Validate httpboot root directory exists when feature is enabled
+ ansible.builtin.fail:
+ msg: "HTTPBoot root directory '{{ foreman_proxy_httpboot_root_dir }}' does not exist on host."
+ when: not _foreman_proxy_httpboot_root_dir.stat.exists
+
- name: Mount httpboot root directory
ansible.builtin.copy:
dest: /etc/containers/systemd/foreman-proxy.container.d/httpboot-volume.conf
content: |
[Container]
Volume={{ foreman_proxy_httpboot_root_dir }}:{{ foreman_proxy_httpboot_root_dir }}:z
@@
- when: _foreman_proxy_httpboot_root_dir.stat.exists
notify:
- Restart Foreman Proxy
- Refresh Foreman Proxy🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/roles/foreman_proxy/tasks/feature/httpboot.yaml` around lines 7 - 17, Add
a deterministic guard before the "Mount httpboot root directory" task: insert a
new task named like "Fail if httpboot root dir missing" that uses
ansible.builtin.fail with a clear message and triggers when not
_foreman_proxy_httpboot_root_dir.stat.exists; keep the existing mount task
unchanged (it can remain conditional on
_foreman_proxy_httpboot_root_dir.stat.exists) so the playbook will fail fast
instead of allowing the httpboot template (settings.d/httpboot.yml.j2) to
advertise a non-existent foreman_proxy_httpboot_root_dir.
There was a problem hiding this comment.
I like fail explicitly if it does not exist rather creating it as tftp feature will create that
There was a problem hiding this comment.
@arvind4501, agreed — keeping the responsibility clear (tftp creates the dir, httpboot only uses it) is the right design. Here is the updated task file reflecting the fail-fast approach:
---
- name: Check if httpboot root directory exists
ansible.builtin.stat:
path: "{{ foreman_proxy_httpboot_root_dir }}"
register: _foreman_proxy_httpboot_root_dir
- name: Fail if httpboot root directory does not exist
ansible.builtin.fail:
msg: >-
HTTPBoot root directory '{{ foreman_proxy_httpboot_root_dir }}' does not
exist on the host. Enable the tftp feature first, or create the directory
manually, before enabling httpboot.
when: not _foreman_proxy_httpboot_root_dir.stat.exists
- name: Mount httpboot root directory
ansible.builtin.copy:
dest: /etc/containers/systemd/foreman-proxy.container.d/httpboot-volume.conf
content: |
[Container]
Volume={{ foreman_proxy_httpboot_root_dir }}:{{ foreman_proxy_httpboot_root_dir }}:z
mode: '0644'
owner: root
group: root
notify:
- Restart Foreman Proxy
- Refresh Foreman ProxyKey changes from the draft:
- The
failtask fires when the directory is absent, giving a clear error message pointing to the dependency on the tftp feature. - The
when: _foreman_proxy_httpboot_root_dir.stat.existsguard is removed from the mount task — it's now redundant because the play will already have aborted before reaching it.
2 participants
Why are you introducing these changes? (Problem description, related links)
Add httpboot feature to foreman proxy
What are the changes introduced in this pull request?
How to test this pull request
Steps to reproduce:
Checklist