USHIFT-6788: explicitly process bootc templates#6486
Conversation
WalkthroughAdded an internal helper Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 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 |
openshift-ci
Bot
added
the
approved
| ofile = os.path.join(BOOTC_IMAGE_DIR, ifile) | ||
| ifile = os.path.join(ipkgdir, ifile) | ||
| run_template_cmd(ifile, ofile, args.dry_run) | ||
| # Process shared bootc templates (USHIFT-6788) |
There was a problem hiding this comment.
Please remove the issue ID from the comment. We do not usually do this unless it's a TODO item.
There was a problem hiding this comment.
This comment has not been addressed.
| run_template_cmd(ifile, ofile, args.dry_run) | ||
| # Process shared bootc templates (USHIFT-6788) | ||
| tpldir = f"{SCRIPTDIR}/../image-blueprints-bootc/templates" | ||
| if os.path.isdir(tpldir): |
There was a problem hiding this comment.
This code is almost a duplicate of a block from process_group function.
Let's refactor it into process_template_files and call from both places?
copejon
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@copejon: This pull request references USHIFT-6788 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 "4.22.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. |
Extract shared gomplate logic for *.template files under a directory into process_template_files(), call from process_group and from the image-blueprints-bootc/templates path. Blueprint templates use skip_existing=not args.force_rebuild and ignore_missing_dir=True to preserve prior skip-when-output-exists behavior without reading FORCE_REBUILD in the helper. Made-with: Cursor
|
@copejon: This pull request references USHIFT-6788 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. |
|
/test e2e-aws-tests-arm |
| ifile = os.path.join(ipkgdir, ifile) | ||
| run_template_cmd(ifile, ofile, args.dry_run) | ||
| # Process shared bootc templates (USHIFT-6788) | ||
| tpldir = f"{SCRIPTDIR}/../image-blueprints-bootc/templates" |
There was a problem hiding this comment.
Should we use os.path.join for consistency instead?
| tpldir = f"{SCRIPTDIR}/../image-blueprints-bootc/templates" | |
| tpldir = os.path.join(SCRIPTDIR, "..", "image-blueprints-bootc", "templates") |
| ignore_missing_dir: if True, no-op when tpldir is absent. | ||
| """ | ||
|
|
||
| if ignore_missing_dir and not os.path.isdir(tpldir): |
There was a problem hiding this comment.
Should we add a log if this happens?
| if not name.endswith(".template"): | ||
| continue | ||
| ofile = os.path.join(BOOTC_IMAGE_DIR, name.removesuffix(".template")) | ||
| if skip_existing and os.path.exists(ofile): |
There was a problem hiding this comment.
Should we add a log if this happens?
There was a problem hiding this comment.
Actually I agree with Giga's comment about the value of the optimizations. I'm removing these vars.
| skip_existing: if True, skip outputs that already exist (pass False when forcing rebuild). | ||
| ignore_missing_dir: if True, no-op when tpldir is absent. |
There was a problem hiding this comment.
I'm not sure we need these two arguments. Let's keep it the way it was because template processing is very very quick.
There was a problem hiding this comment.
Looking back at it, I agree. Will remove.
|
@copejon: This pull request references USHIFT-6788 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. |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/bin/pyutils/build_bootc_images.py`:
- Around line 221-228: process_template_files currently lists tpldir without
handling a missing directory; add an optional parameter ignore_missing_dir=False
(matching the caller) and either check os.path.isdir(tpldir) and return early
when ignore_missing_dir is True or catch FileNotFoundError around
os.listdir(tpldir) and return silently when ignore_missing_dir is True; keep
behavior unchanged when ignore_missing_dir is False by re-raising the error.
Reference: process_template_files(tpldir, dry_run), BOOTC_IMAGE_DIR, and
run_template_cmd.
- Around line 663-668: The call to process_template_files(tpldir, args.dry_run,
skip_existing=..., ignore_missing_dir=...) passes keyword args that no longer
exist; update the call to match the current signature by removing skip_existing
and ignore_missing_dir and simply call process_template_files(tpldir,
args.dry_run). If the template directory may be absent and should be silently
skipped, add a guard before the call (e.g., check os.path.isdir(tpldir)) and
skip calling process_template_files when the directory doesn't exist; if the
original skip_existing behavior is still required, implement that handling at
the call site or restore it inside process_template_files.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: ee8b7e87-2f11-4957-8a4a-09915fe99fd3
📒 Files selected for processing (1)
test/bin/pyutils/build_bootc_images.py
|
|
||
| tpldir = os.path.join(SCRIPTDIR, "..", "image-blueprints-bootc", "templates") | ||
| process_template_files( | ||
| tpldir, args.dry_run, | ||
| skip_existing=not args.force_rebuild, | ||
| ignore_missing_dir=True) |
There was a problem hiding this comment.
Function call passes arguments that don't exist in the signature.
process_template_files() is defined with only (tpldir, dry_run) parameters (lines 221-222), but this call passes skip_existing and ignore_missing_dir keyword arguments. This will raise TypeError at runtime.
Per the commit message, these parameters were intentionally removed. Update the call to match:
Proposed fix
tpldir = os.path.join(SCRIPTDIR, "..", "image-blueprints-bootc", "templates")
- process_template_files(
- tpldir, args.dry_run,
- skip_existing=not args.force_rebuild,
- ignore_missing_dir=True)
+ if os.path.isdir(tpldir):
+ process_template_files(tpldir, args.dry_run)If the directory might not exist and should be silently skipped, add a guard. If skip_existing logic is still needed, either restore it in process_template_files() or handle it at the call site.
📝 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.
| tpldir = os.path.join(SCRIPTDIR, "..", "image-blueprints-bootc", "templates") | |
| process_template_files( | |
| tpldir, args.dry_run, | |
| skip_existing=not args.force_rebuild, | |
| ignore_missing_dir=True) | |
| tpldir = os.path.join(SCRIPTDIR, "..", "image-blueprints-bootc", "templates") | |
| if os.path.isdir(tpldir): | |
| process_template_files(tpldir, args.dry_run) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/bin/pyutils/build_bootc_images.py` around lines 663 - 668, The call to
process_template_files(tpldir, args.dry_run, skip_existing=...,
ignore_missing_dir=...) passes keyword args that no longer exist; update the
call to match the current signature by removing skip_existing and
ignore_missing_dir and simply call process_template_files(tpldir, args.dry_run).
If the template directory may be absent and should be silently skipped, add a
guard before the call (e.g., check os.path.isdir(tpldir)) and skip calling
process_template_files when the directory doesn't exist; if the original
skip_existing behavior is still required, implement that handling at the call
site or restore it inside process_template_files.
…ever missing, and b) processed very fast, thus don't require optimization
|
@copejon: This pull request references USHIFT-6788 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/bin/pyutils/build_bootc_images.py (1)
223-228: Make template rendering order deterministic.
os.listdir()is filesystem-order dependent. Sorting here avoids non-deterministic render order and precedence drift when filenames collide.Proposed patch
def process_template_files(tpldir, dry_run): """Expand *.template files under tpldir into BOOTC_IMAGE_DIR via gomplate.""" - for name in os.listdir(tpldir): + for name in sorted(os.listdir(tpldir), key=lambda i: (len(i), i)): if not name.endswith(".template"): continue ofile = os.path.join(BOOTC_IMAGE_DIR, name.removesuffix(".template")) ifile = os.path.join(tpldir, name) run_template_cmd(ifile, ofile, dry_run)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/bin/pyutils/build_bootc_images.py` around lines 223 - 228, The loop over os.listdir(tpldir) is non-deterministic; change it to iterate over sorted(os.listdir(tpldir)) (or equivalent sorted sequence) so template files are rendered in a stable, lexicographic order—update the loop that currently uses os.listdir(tpldir) and still calls run_template_cmd(ifile, ofile, dry_run) while using tpldir and BOOTC_IMAGE_DIR to compute ifile/ofile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/bin/pyutils/build_bootc_images.py`:
- Around line 223-228: The loop over os.listdir(tpldir) is non-deterministic;
change it to iterate over sorted(os.listdir(tpldir)) (or equivalent sorted
sequence) so template files are rendered in a stable, lexicographic order—update
the loop that currently uses os.listdir(tpldir) and still calls
run_template_cmd(ifile, ofile, dry_run) while using tpldir and BOOTC_IMAGE_DIR
to compute ifile/ofile.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: ed94338a-b921-4637-9cb9-86fb412c176b
📒 Files selected for processing (1)
test/bin/pyutils/build_bootc_images.py
|
/test e2e-aws-tests-bootc-periodic-el10 |
|
@copejon: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/lgtm |
|
@ggiguash: once the present PR merges, I will cherry-pick it on top of 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 kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: copejon, ggiguash 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 |
|
/verified by ci |
openshift-ci-robot
added
the
verified
|
@ggiguash: This PR has been marked as verified by 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. |
|
@ggiguash: new pull request created: #6513 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 kubernetes-sigs/prow repository. |
5 participants
Summary
image-blueprints-bootc/templates/directoryFixes: USHIFT-6788
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit