Repository navigation
OSAC-1641: fix double role prefix in bare metal instance playbook - #359
Conversation
|
@adriengentil: This pull request references OSAC-1641 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Warning Review limit reached
More reviews will be available in 54 minutes and 15 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (1)
WalkthroughAdds default Fedora 44 image variables to the ChangesMetal3 provisioning role hardening and role name fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 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 |
templateID already contains the fully qualified role name (e.g. osac.templates.bm_host_metal3_provisioning), so prepending osac.templates. caused a lookup failure. Use the templateID directly, matching the convention in playbook_osac_create_compute_instance.yml. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Adrien Gentil <agentil@redhat.com>
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Adrien Gentil <agentil@redhat.com>
81eb931 to
6bdce18
Compare
…sioning role Without defaults, imageURL and imageChecksum must be passed explicitly in templateParameters. The fulfillment-controller only forwards sshKey today, so defaults ensure the role can provision without requiring callers to supply image parameters. Defaults can be overridden per-instance via templateParameters. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Adrien Gentil <agentil@redhat.com>
When externalHostID is not set there is no BMH to deprovision. Wrapping all tasks in a block conditioned on externalHostID prevents NoneType split errors during deletion of unprovisioned instances. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Adrien Gentil <agentil@redhat.com>
|
@adriengentil: This pull request references OSAC-1641 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
default('') only substitutes undefined variables, not null values.
default('', true) also substitutes falsy values including None.
Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Adrien Gentil <agentil@redhat.com>
![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
🤖 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
`@collections/ansible_collections/osac/templates/roles/bm_host_metal3_provisioning/defaults/main.yaml`:
- Around line 2-4: The hardcoded value for bm_host_metal3_default_image_checksum
lacks a documented, verifiable source which creates a supply-chain security
risk. Either add a clear comment above bm_host_metal3_default_image_checksum
explaining where the checksum value originated from (e.g., official Fedora build
artifacts, signed manifest URL, or manual verification), or refactor the
configuration to fetch the checksum dynamically at runtime from an official
Fedora checksum source instead of hardcoding it. Ensure the chosen approach is
documented so future maintainers understand the validation chain.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 3f7b84ea-ae1e-407a-b353-6b00ff8b5ac4
📒 Files selected for processing (5)
collections/ansible_collections/osac/templates/roles/bm_host_metal3_provisioning/defaults/main.yamlcollections/ansible_collections/osac/templates/roles/bm_host_metal3_provisioning/tasks/create.yamlcollections/ansible_collections/osac/templates/roles/bm_host_metal3_provisioning/tasks/delete.yamlplaybook_osac_create_bare_metal_instance.ymlplaybook_osac_delete_bare_metal_instance.yml
| bm_host_metal3_default_image_url: >- | ||
| https://download.fedoraproject.org/pub/fedora/linux/releases/44/Cloud/x86_64/images/Fedora-Cloud-Base-Generic-44-1.7.x86_64.qcow2 | ||
| bm_host_metal3_default_image_checksum: "28680fe5b371a5a82ebf43a31926e086a168e59949d03969c5093e7071f90b7f" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Fedora 44 image URL accessibility and checksum
URL="https://download.fedoraproject.org/pub/fedora/linux/releases/44/Cloud/x86_64/images/Fedora-Cloud-Base-Generic-44-1.7.x86_64.qcow2"
EXPECTED_CHECKSUM="28680fe5b371a5a82ebf43a31926e086a168e59949d03969c5093e7071f90b7f"
# Check if URL is accessible
echo "Checking URL accessibility..."
HTTP_STATUS=$(curl -I -s -o /dev/null -w "%{http_code}" "$URL")
echo "HTTP Status: $HTTP_STATUS"
if [ "$HTTP_STATUS" -eq 200 ]; then
echo "✓ URL is accessible"
# Verify checksum from Fedora's checksum file
echo "Fetching official checksum..."
CHECKSUM_URL="https://download.fedoraproject.org/pub/fedora/linux/releases/44/Cloud/x86_64/images/Fedora-Cloud-44-1.7-x86_64-CHECKSUM"
curl -s "$CHECKSUM_URL" | grep -i "Fedora-Cloud-Base-Generic.*\.qcow2" || echo "Checksum file not found or pattern not matched"
else
echo "✗ URL returned status $HTTP_STATUS"
fiRepository: osac-project/osac-aap
Length of output: 138
🏁 Script executed:
#!/bin/bash
# Better verification with redirect following and official checksum validation
URL="https://download.fedoraproject.org/pub/fedora/linux/releases/44/Cloud/x86_64/images/Fedora-Cloud-Base-Generic-44-1.7.x86_64.qcow2"
EXPECTED_CHECKSUM="28680fe5b371a5a82ebf43a31926e086a168e59949d03969c5093e7071f90b7f"
CHECKSUM_URL="https://download.fedoraproject.org/pub/fedora/linux/releases/44/Cloud/x86_64/images/CHECKSUM"
echo "=== Checking URL with redirect support ==="
HTTP_STATUS=$(curl -I -s -L -o /dev/null -w "%{http_code}" "$URL")
echo "Final HTTP Status (following redirects): $HTTP_STATUS"
if [ "$HTTP_STATUS" -eq 200 ]; then
echo "✓ URL resolves successfully (including redirects)"
else
echo "✗ URL returned status $HTTP_STATUS"
fi
echo ""
echo "=== Fetching official Fedora checksum file ==="
CHECKSUM_CONTENT=$(curl -s "$CHECKSUM_URL" 2>/dev/null || echo "NOT_FOUND")
if [ "$CHECKSUM_CONTENT" != "NOT_FOUND" ]; then
echo "Checksum file found. Searching for image entry..."
echo "$CHECKSUM_CONTENT" | grep -i "Fedora-Cloud-Base-Generic-44-1.7.*\.qcow2" || echo "Image pattern not found in checksum file"
# Try alternate patterns
echo ""
echo "Checking all SHA256 entries:"
echo "$CHECKSUM_CONTENT" | grep -i "sha256" | head -5
else
echo "Could not fetch checksum file from: $CHECKSUM_URL"
fiRepository: osac-project/osac-aap
Length of output: 372
🏁 Script executed:
#!/bin/bash
CHECKSUM_BASE="https://download.fedoraproject.org/pub/fedora/linux/releases/44/Cloud/x86_64/images"
IMAGE_NAME="Fedora-Cloud-Base-Generic-44-1.7.x86_64.qcow2"
EXPECTED_CHECKSUM="28680fe5b371a5a82ebf43a31926e086a168e59949d03969c5093e7071f90b7f"
echo "=== Trying various checksum file formats ==="
for VARIANT in "CHECKSUM" "CHECKSUM.txt" "CHECKSUM.sha256" "sha256sum" "sha256sum.txt" "CHECKSUMS"; do
URL="${CHECKSUM_BASE}/${VARIANT}"
echo ""
echo "Attempting: $VARIANT"
RESPONSE=$(curl -s -o /dev/null -w "%{http_code}" "$URL")
if [ "$RESPONSE" = "200" ]; then
echo "✓ Found: $URL"
echo "Content preview:"
curl -s "$URL" | head -20
else
echo "✗ Not found (HTTP $RESPONSE)"
fi
done
echo ""
echo "=== Checking image metadata ==="
echo "Expected checksum: $EXPECTED_CHECKSUM"
echo "Image URL: ${CHECKSUM_BASE}/${IMAGE_NAME}"
# Try to get Content-Length or Last-Modified headers
echo ""
echo "Image metadata:"
curl -I -s -L "${CHECKSUM_BASE}/${IMAGE_NAME}" | grep -E "Content-Length|Last-Modified|Content-Type"Repository: osac-project/osac-aap
Length of output: 786
Document the source of the hardcoded checksum or verify it can be validated.
The URL is accessible and the image exists, but the SHA256 checksum cannot be verified against Fedora's official sources—standard checksum files (CHECKSUM, sha256sum, etc.) are not published at the Fedora release mirror. Hardcoding a checksum without a documented, verifiable source is a supply-chain risk. Either:
- Document where this checksum came from (official Fedora build, signed manifest, etc.)
- Remove the checksum and fetch it from a documented official source at runtime
- Provide an alternative validation method
🤖 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
`@collections/ansible_collections/osac/templates/roles/bm_host_metal3_provisioning/defaults/main.yaml`
around lines 2 - 4, The hardcoded value for
bm_host_metal3_default_image_checksum lacks a documented, verifiable source
which creates a supply-chain security risk. Either add a clear comment above
bm_host_metal3_default_image_checksum explaining where the checksum value
originated from (e.g., official Fedora build artifacts, signed manifest URL, or
manual verification), or refactor the configuration to fetch the checksum
dynamically at runtime from an official Fedora checksum source instead of
hardcoding it. Ensure the chosen approach is documented so future maintainers
understand the validation chain.
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adriengentil, carbonin The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
Summary
playbook_osac_create_bare_metal_instance.ymlwas prependingosac.templates.tobare_metal_instance.spec.templateID, which already contains the fully qualified role name (e.g.osac.templates.bm_host_metal3_provisioning)role 'osac.templates.osac.templates.bm_host_metal3_provisioning' was not found{{ bare_metal_instance.spec.templateID }}directly, matching the convention inplaybook_osac_create_compute_instance.ymlJira
https://redhat.atlassian.net/browse/OSAC-1641
Test plan
ansible-lintpasses🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements