Enable bootc image builder on more RHEL releases#6320
Enable bootc image builder on more RHEL releases#6320smitterl merged 1 commit intoautotest:masterfrom
Conversation
chunfuwen
force-pushed
the
enable_bootc_on_rhel9.7_10.1
branch
from
f700186 to
bc010a1
Compare
chunfuwen
force-pushed
the
enable_bootc_on_rhel9.7_10.1
branch
from
bc010a1 to
d623d90
Compare
chunfuwen
force-pushed
the
enable_bootc_on_rhel9.7_10.1
branch
3 times, most recently
from
7c72801 to
5c30738
Compare
chunfuwen
force-pushed
the
enable_bootc_on_rhel9.7_10.1
branch
from
5c30738 to
137e8be
Compare
chunfuwen
force-pushed
the
enable_bootc_on_rhel9.7_10.1
branch
from
137e8be to
852b6cd
Compare
smitterl
left a comment
smitterl
left a comment
There was a problem hiding this comment.
Please update you header/message according to our style guide https://avocado-framework.readthedocs.io/en/latest/guides/contributor/chapters/styleguides.html#commit-style-guide
Please also have a look at my other comments.
Thank you
virttools/tests/cfg/bootc_image_builder/bootc_disk_image_build.cfg
Outdated
Show resolved
Hide resolved
virttools/tests/cfg/bootc_image_builder/bootc_disk_image_build.cfg
Outdated
Show resolved
Hide resolved
chunfuwen
force-pushed
the
enable_bootc_on_rhel9.7_10.1
branch
from
852b6cd to
c0bb22f
Compare
|
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 support for RHEL 9.7/9.8 and RHEL 10.1/10.2 across build/install tooling and tests, expands test/config variants and metadata, and switches the local registry image to ghcr.io/osbuild/bootc-image-builder/registry:2. Podman invocations now include --quiet and a default --log-level=info; when --use-librepo is enabled for anaconda-iso builds the podman log level is changed to --log-level=error and PODMAN_LOG_LEVEL is set to "error". create_and_build_container_file recognizes new RHEL repo filename/prefixes. Test auth-condition lists were extended to cover the new bib variants. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as User CLI
participant Utils as build_utils
participant Podman as Podman
participant Registry as LocalRegistry
participant Auth as AuthSetup
CLI->>Utils: start image build (bib_ref)
alt bib_ref ∈ {rhel_9.7_nightly_bib, rhel_9.8_nightly_bib, rhel_10.1_bib, rhel_10.2_bib, ...}
Utils->>Auth: create auth.json & podman login
end
Utils->>Registry: ensure_registry(image=ghcr.io/osbuild/bootc-image-builder/registry:2)
Registry-->>Utils: registry ready
Utils->>Podman: podman build (--quiet, --log-level=info by default, --use-librepo if anaconda-iso)
alt --use-librepo enabled
Note over Podman,Utils: replace --log-level=info with --log-level=error\nand set PODMAN_LOG_LEVEL="error"
end
Podman-->>Utils: image built
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 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)
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
provider/bootc_image_builder/bootc_image_build_utils.py (3)
88-90: Bug: uses built-intypeinstead ofdisk_image_typeThis condition never matches and shadows the built-in. Fix variable name.
- if type in ['anaconda-iso'] and "9.4" not in bib_image_url and "9.5" not in bib_image_url: + if disk_image_type in ['anaconda-iso'] and "9.4" not in bib_image_url and "9.5" not in bib_image_url:
1205-1210: Security: avoid eval() on external data
eval(ret)on skopeo output is unsafe. Parse as JSON; handle null.- bootc_meta_info_dict = eval(ret) + try: + bootc_meta_info_dict = json.loads(ret) if ret and ret.strip() != "null" else {} + except json.JSONDecodeError: + bootc_meta_info_dict = {}
832-835: Logic error: string membership check
if result not in "poweredOn":is wrong; use equality.- if result not in "poweredOn": + if result != "poweredOn":virttools/tests/src/bootc_image_builder/bootc_disk_image_build.py (1)
69-71: Safer parsing for aws_config_dictCurrent code uses eval() elsewhere; prefer json or ast.literal_eval.
- aws_config_dict = eval(params.get("aws_config_dict", '{}')) + import ast + aws_config_dict = ast.literal_eval(params.get("aws_config_dict", '{}'))Also applies to: 118-121
virttools/tests/src/bootc_image_builder/bootc_disk_image_install.py (1)
84-86: Safer parsing for aws_config_dictUse ast.literal_eval to avoid code execution.
- aws_config_dict = eval(params.get("aws_config_dict", '{}')) + import ast + aws_config_dict = ast.literal_eval(params.get("aws_config_dict", '{}'))Also applies to: 119-121
virttools/tests/cfg/bootc_image_builder/bootc_disk_image_install.cfg (1)
165-166: Sensitive secret committed—remove immediately
podman_quay_password = "doudou303"appears to be a real credential. Replace with a placeholder or pull from environment/CI secrets; rotate the credential.
Suggested fix:
- Remove concrete passwords from repo, use variables (e.g., ${PODMAN_QUAY_PASSWORD}) and document injection via CI.
♻️ Duplicate comments (4)
virttools/tests/cfg/bootc_image_builder/bootc_disk_image_build.cfg (2)
48-51: Scope of centos10-only list—revisit 10.0 exclusionYou exclude rhel_10.0 here. If intentional, add rationale; otherwise re-include 10.0.
219-227: Do not commit internal compose URLs
download.eng.bos.redhat.comshould not be in upstream config; use a placeholder (e.g., LATEST_RHEL_10_1_COMPOSE_URL) configured in CI/secrets. This was raised before.
Suggested change:
- Replace hardcoded BOS URLs with a token/var and document how CI supplies it.
provider/bootc_image_builder/bootc_image_build_utils.py (1)
1338-1338: Registry image change: parameterize and pin; add fallbackSwitching to ghcr.io/osbuild/bootc-image-builder/registry:2 is fine, but make it configurable, and prefer digest pinning; optionally fall back to registry:2 if pull fails. Also answers prior reviewer’s question.
Apply:
- subprocess.run([ - "podman", "run", "-d", "-p", "5000:5000", "--restart", "always", "--name", "registry", "ghcr.io/osbuild/bootc-image-builder/registry:2" - ], check=True, capture_output=True) + registry_image = params.get( + "local_registry_image", + "ghcr.io/osbuild/bootc-image-builder/registry:2" + ) + try: + subprocess.run([ + "podman", "run", "-d", "-p", "5000:5000", "--restart", "always", + "--name", "registry", registry_image + ], check=True, capture_output=True) + except subprocess.CalledProcessError: + # fallback to default if custom image fails + subprocess.run([ + "podman", "run", "-d", "-p", "5000:5000", "--restart", "always", + "--name", "registry", "registry:2" + ], check=True, capture_output=True)Optionally pin digest (please provide digest):
What is the current digest for ghcr.io/osbuild/bootc-image-builder/registry:2?virttools/tests/cfg/bootc_image_builder/bootc_disk_image_install.cfg (1)
211-219: Do not commit internal compose URLsSame BOS URL issue as build cfg; replace with placeholder and supply via CI.
🧹 Nitpick comments (3)
provider/bootc_image_builder/bootc_image_build_utils.py (3)
600-608: Good: adds 9.7 and 10.1 repo detectionLooks consistent with prior 9.x/10.0 branches. Consider deriving repo_prefix from the filename to avoid future copy/paste blocks.
1320-1323: Minor: wrong command used and typo in log
list_imageis unused;cmdis re-run. Also "pomdman" typo.- list_image = "podman images localhost:5000/{image_name}" - ret = process.run(cmd, timeout=40, verbose=True, ignore_status=True, shell=True).stdout_text - LOG.debug(f"pomdman list image output is : {ret}") + list_image = f"podman images localhost:5000/{image_name}" + ret = process.run(list_image, timeout=40, verbose=True, ignore_status=True, shell=True).stdout_text + LOG.debug(f"podman list image output is : {ret}")
119-119: Hard-coded --progress=verbose is valid but could be made configurableThe value
verboseis indeed supported by bootc-image-builder, so the current code is not broken. However, making it configurable via dargs improves flexibility. Consider adopting the suggested change to allow users to specify progress behavior (valid values:verbose,term,debug):- cmd += " --progress=verbose" + progress = dargs.get("progress") + if progress: + cmd += f" --progress={progress}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
provider/bootc_image_builder/bootc_image_build_utils.py(3 hunks)virttools/tests/cfg/bootc_image_builder/bootc_disk_image_build.cfg(9 hunks)virttools/tests/cfg/bootc_image_builder/bootc_disk_image_install.cfg(9 hunks)virttools/tests/src/bootc_image_builder/bootc_disk_image_build.py(1 hunks)virttools/tests/src/bootc_image_builder/bootc_disk_image_install.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.11
- GitHub Check: Python 3.8
- GitHub Check: Python 3.12
- GitHub Check: Python 3.9
🔇 Additional comments (23)
virttools/tests/src/bootc_image_builder/bootc_disk_image_build.py (1)
84-86: Auth gating list updated—LGTMNew bib_refs included for auth.json and login. Ensure corresponding cfg entries exist (they do in this PR).
virttools/tests/src/bootc_image_builder/bootc_disk_image_install.py (3)
90-93: Adds rhel_9.7_nightly_bib local_image knobs—LGTMMatches build-side additions.
96-98: Auth gating list updated—LGTMrhel_9.7_nightly_bib and rhel_10.1_bib covered.
100-101: build_container mapping for 10.1—LGTMKeeps upstream_bib aligned with latest RHEL bootc.
virttools/tests/cfg/bootc_image_builder/bootc_disk_image_build.cfg (9)
32-35: OK: enable kickstart and FS sizing for 9.6/9.7 anaconda local_imageConsistent with other variants.
55-73: Fedora blocks updated—LGTMfedora_legacy split and s390 image bump to 42 looks fine.
81-88: Nightly build_container mapping—LGTMAdds 9.6/9.7/10.1; consistent with upstream_bib pinning.
109-114: Add rhel_9.7_nightly—LGTMIncludes lvm option and blocks anaconda-iso.
121-126: Add rhel_10.1_nightly—LGTMTLS disabled and no anaconda-iso consistent with 10.0 pattern.
199-207: Add rhel_9.7_nightly_bib—LGTMAuth and local anaconda mapping OK.
239-241: AMI scoping narrowed—verify intentLimiting AWS runs to a single path reduces coverage; confirm this is desired for CI time/cost.
254-255: OK: add no s390-virtio to anaconda-isoMatches capabilities.
264-265: Variant filtering updates—LGTMLimiting vhd/gce to upstream_bib, 9.7 nightly, 10.1 bib seems deliberate; keep 10.0 if still supported.
Also applies to: 270-270
virttools/tests/cfg/bootc_image_builder/bootc_disk_image_install.cfg (10)
43-44: Install gating—LGTMAdds rhel_10.1_bib and rhel_9.7_nightly_bib to centos10 path.
52-72: Fedora install blocks—LGTMLegacy/latest split and s390 tag bump to 42 OK.
85-93: Local rhel_9.7_nightly_bib knobs—LGTMEnables FIPS and repo flag.
100-101: build_container mapping for 10.1—LGTMMirrors build config.
117-120: Add rhel_9.7_nightly—LGTMIntentional TLS disabled and no anaconda-iso.
127-130: Add rhel_10.1_nightly—LGTMConsistent with 10.0 nightly pattern.
191-199: Add rhel_9.7_nightly_bib—LGTMAuth and local anaconda mapping OK.
236-247: AWS config dict overrides—LGTMExplicitly disabling cloud runs for certain combinations is fine.
268-268: OK: add no s390-virtio to anaconda-isoMatches capabilities.
278-279: Variant filtering updates—LGTMLimit vhd/gce to upstream_bib, 9.7 nightly, 10.1 bib; confirm 10.0 exclusion is intended.
Also applies to: 284-284
chunfuwen
force-pushed
the
enable_bootc_on_rhel9.7_10.1
branch
from
c0bb22f to
c5663eb
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
provider/bootc_image_builder/bootc_image_build_utils.py (4)
88-90: Bug: undefinedtypeused; should bedisk_image_typeThis path never triggers as written. Replace
typewithdisk_image_type.- if type in ['anaconda-iso'] and "9.4" not in bib_image_url and "9.5" not in bib_image_url: + if disk_image_type in ['anaconda-iso'] and "9.4" not in bib_image_url and "9.5" not in bib_image_url:
139-141: Avoid passing passwords on the CLI
podman login -pexposes secrets via process list and logs. Use--password-stdinor an authfile.-def podman_login(podman_username, podman_password, registry): +def podman_login(podman_username, podman_password, registry): ... - command = "sudo podman login -u='%s' -p='%s' %s " % (podman_username, podman_password, registry) - process.run( - command, timeout=60, verbose=True, ignore_status=False, shell=True) + # safer: read password from stdin + cmd = f"sudo podman login -u='{podman_username}' --password-stdin {registry}" + process.run(f"printf %s | {cmd}", timeout=60, verbose=False, ignore_status=False, shell=True)
652-653: Dockerfile directive typo breaks custom_repo builds
Run dnf clean allshould beRUN. This causes a build failure whencustom_repois set.- Run dnf clean all + RUN dnf clean all
1318-1320: Fix debug listing typo
list_imageis never executed;cmdis reused. Execute the intended command and log it.- list_image = "podman images localhost:5000/{image_name}" - ret = process.run(cmd, timeout=40, verbose=True, ignore_status=True, shell=True).stdout_text - LOG.debug(f"pomdman list image output is : {ret}") + list_cmd="podman images localhost:5000/%s" % image_name + out = process.run(list_cmd, timeout=40, verbose=True, ignore_status=True, shell=True).stdout_text + LOG.debug("podman list image output: %s", out)virttools/tests/cfg/bootc_image_builder/bootc_disk_image_install.cfg (1)
160-167: Sanitize credentials from repoReal usernames/passwords appear present. Replace with placeholders and load secrets from CI/runner env or Avocado var files not committed.
- podman_redhat_username = "11080659|chwen" - podman_redhat_password = "podman_redhat_password" + podman_redhat_username = "REDACTED_IN_REPO" + podman_redhat_password = "REDACTED_IN_REPO" ... - podman_quay_username = "wenbaoxin" - podman_quay_password = "doudou303" + podman_quay_username = "REDACTED_IN_REPO" + podman_quay_password = "REDACTED_IN_REPO"
♻️ Duplicate comments (7)
virttools/tests/cfg/bootc_image_builder/bootc_disk_image_build.cfg (4)
48-51: Don’t drop 10.0 coverage in centos10 block without deprecation plan
only ... rhel_10.1_bib ... rhel_9.7_nightly_bib ...omits 10.0. If 10.0 remains supported, keep it in the matrix.Proposed tweak:
- only upstream_bib..use_config_json..tls_verify_enable, rhel_10.1_bib..use_config_json..tls_verify_enable, rhel_9.7_nightly_bib..use_config_json..tls_verify_enable, rhel_9.4_nightly_bib..use_config_json..tls_verify_enable + only upstream_bib..use_config_json..tls_verify_enable, rhel_10.1_bib..use_config_json..tls_verify_enable, rhel_10.0_bib..use_config_json..tls_verify_enable, rhel_9.7_nightly_bib..use_config_json..tls_verify_enable, rhel_9.4_nightly_bib..use_config_json..tls_verify_enable
254-254: Why “no s390-virtio” for anaconda-iso?New exclusion needs a short rationale (installer limitation? missing driver?). Otherwise it silently narrows coverage.
264-270: vhd/gce outputs exclude 10.0—intentional?
only upstream_bib, rhel_9.7_nightly_bib, rhel_10.1_bibdrops 10.0. Keep 10.0 until officially removed, or document deprecation.- only upstream_bib, rhel_9.7_nightly_bib, rhel_10.1_bib + only upstream_bib, rhel_9.7_nightly_bib, rhel_10.1_bib, rhel_10.0_bib
218-227: Remove internal BOS URL; use placeholder/CI secret
compose_urlpoints todownload.eng.bos.redhat.comand embeds a compose ID. Avoid upstream exposure; inject via CI or placeholder (e.g., LATEST_RHEL_10_1_COMPOSE_URL).- compose_url = "https://download.eng.bos.redhat.com/rhel-10/nightly/RHEL-10-Public-Beta/RHEL-10.1-20250522.76" + compose_url = "LATEST_RHEL_10_1_COMPOSE_URL"provider/bootc_image_builder/bootc_image_build_utils.py (1)
1334-1337: Hardcoded GHCR registry image — parameterize or keep docker.io fallbackChange answers a supply issue, but hardcoding reduces flexibility. Make the image overridable via params/env, with fallback to
docker.io/library/registry:2.- subprocess.run([ - "podman", "run", "-d", "-p", "5000:5000", "--restart", "always", "--name", "registry", "ghcr.io/osbuild/bootc-image-builder/registry:2" - ], check=True, capture_output=True) + registry_image = os.getenv("LOCAL_REGISTRY_IMAGE", "ghcr.io/osbuild/bootc-image-builder/registry:2") + subprocess.run([ + "podman", "run", "-d", "-p", "5000:5000", "--restart", "always", + "--name", "registry", registry_image + ], check=True, capture_output=True)virttools/tests/cfg/bootc_image_builder/bootc_disk_image_install.cfg (2)
43-44: Don’t remove 10.0 from centos10 path without deprecation planLike build cfg, this narrows coverage. Keep 10.0 unless explicitly unsupported.
- only upstream_bib, rhel_10.1_bib, rhel_9.7_nightly_bib + only upstream_bib, rhel_10.1_bib, rhel_10.0_bib, rhel_9.7_nightly_bib
278-284: vhd/gce: include 10.0 if still supportedMirror build cfg suggestion to keep 10.0 until removal is intentional.
- only upstream_bib, rhel_9.7_nightly_bib, rhel_10.1_bib + only upstream_bib, rhel_9.7_nightly_bib, rhel_10.1_bib, rhel_10.0_bib
🧹 Nitpick comments (3)
virttools/tests/cfg/bootc_image_builder/bootc_disk_image_build.cfg (2)
72-72: Pinning s390x to Fedora 42 may drift from “latest”s390-virtio uses
:42while others use:latest. Consider aligning or documenting why s390x must be pinned.
80-87: Confirm intent: upstream_bib shares RHEL 10.1 build_container
rhel_10.1_bib, upstream_bib:sets the same RHEL 10.1 base. If upstream_bib is meant to be distro‑agnostic (CentOS path), keep its base separate.- rhel_10.1_bib, upstream_bib: - build_container = "registry.stage.redhat.io/rhel10/rhel-bootc:10.1" + rhel_10.1_bib: + build_container = "registry.stage.redhat.io/rhel10/rhel-bootc:10.1" +# upstream_bib left unchanged or set explicitly to its desired basevirttools/tests/cfg/bootc_image_builder/bootc_disk_image_install.cfg (1)
71-71: s390x pinned to Fedora 42—document or align with latestSame concern as build cfg: explain pin or make consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
provider/bootc_image_builder/bootc_image_build_utils.py(2 hunks)virttools/tests/cfg/bootc_image_builder/bootc_disk_image_build.cfg(9 hunks)virttools/tests/cfg/bootc_image_builder/bootc_disk_image_install.cfg(9 hunks)virttools/tests/src/bootc_image_builder/bootc_disk_image_build.py(1 hunks)virttools/tests/src/bootc_image_builder/bootc_disk_image_install.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- virttools/tests/src/bootc_image_builder/bootc_disk_image_install.py
- virttools/tests/src/bootc_image_builder/bootc_disk_image_build.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.12
- GitHub Check: Python 3.9
- GitHub Check: Python 3.11
- GitHub Check: Python 3.8
🔇 Additional comments (15)
virttools/tests/cfg/bootc_image_builder/bootc_disk_image_build.cfg (5)
32-35: 9.7 BIB anaconda-iso hook looks fineAddition aligns with 9.6 pattern; no issues spotted.
54-61: Fedora legacy rename is consistentRename/readjustments look good; selection and roofs override preserved.
108-114: 9.7 nightly block LGTMValues mirror 9.6 nightly; includes LVM flag for qcow.
121-126: 10.1 nightly block LGTMMatches 10.0 nightly semantics with TLS disabled.
198-207: Add 9.7 nightly BIB: looks correctStage registry, creds placeholders, and local_image mapping added consistently.
provider/bootc_image_builder/bootc_image_build_utils.py (2)
598-601: Add 9.7 repo mapping — OKMapping for rhel-9.7 repo/prefix is consistent with prior versions.
604-607: Add 10.1 repo mapping — OKMapping for rhel-10.1 matches existing pattern.
virttools/tests/cfg/bootc_image_builder/bootc_disk_image_install.cfg (8)
51-63: Fedora legacy rename looks goodMatrix and per‑format overrides retained.
85-93: 9.7 local_image + FIPS flags OKConsistent with 9.5/9.6; good.
100-101: 10.1 base image for local_image/upstream_bibSame intent question as build cfg: should upstream_bib share 10.1 base?
116-120: 9.7 nightly entry LGTMTLS disabled and anaconda excluded as expected.
126-130: 10.1 nightly entry LGTMMatches 10.0 nightly semantics.
191-199: 9.7 nightly BIB block OKStage creds + local_image mapping consistent.
236-247: AWS config dict expansions — OKNew bibs added and others gated off appropriately.
268-268: Why exclude s390x for anaconda-iso here?Add a brief comment/rationale; otherwise this silently reduces test coverage.
virttools/tests/cfg/bootc_image_builder/bootc_disk_image_install.cfg
Outdated
Show resolved
Hide resolved
chunfuwen
force-pushed
the
enable_bootc_on_rhel9.7_10.1
branch
from
c5663eb to
88993c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
virttools/tests/cfg/bootc_image_builder/bootc_disk_image_build.cfg (3)
48-48: Clarify RHEL 10.0 variant removal.This line now restricts centos10 testing to newer bib variants (rhel_10.1_bib, rhel_9.7_nightly_bib), effectively removing rhel_10.0_bib coverage. Since RHEL 10.0 will be supported for an extended period, document why coverage is being dropped or confirm this is intentional.
Is RHEL 10.0 intentionally being phased out of test coverage? If so, consider adding a comment explaining the rationale.
111-112: Document the LVM disk partitions requirement.The
enable_lvm_disk_partitionsoption is newly introduced for rhel_9.7_nightly_bib qcow builds. Per past review, clarify why this became necessary—does RHEL 9.7 default to LVM layouts, or is this testing a specific configuration?Add a comment explaining when/why LVM partitioning is required for 9.7.
218-227: Remove internal BOS URL and compose ID.The
compose_urlcontains an internal Red Hat engineering host (download.eng.bos.redhat.com) and a specific compose identifier that should not be exposed in upstream repositories. Replace with a placeholder (e.g.,LATEST_RHEL_10_1_COMPOSE_URL) that CI can inject at runtime.Apply this diff:
- compose_url = "https://download.eng.bos.redhat.com/rhel-10/nightly/RHEL-10-Public-Beta/RHEL-10.1-20250522.76" + compose_url = "LATEST_RHEL_10_1_COMPOSE_URL"virttools/tests/cfg/bootc_image_builder/bootc_disk_image_install.cfg (2)
210-219: Remove internal BOS URL and compose ID.Same as in the build config: the
compose_urlexposesdownload.eng.bos.redhat.comand a specific compose identifier. Replace with a CI-injected placeholder.Apply this diff:
- compose_url = "https://download.eng.bos.redhat.com/rhel-10/nightly/RHEL-10-Public-Beta/RHEL-10.1-20250522.76" + compose_url = "LATEST_RHEL_10_1_COMPOSE_URL"
248-248: Fix inconsistent Fedora alias after rename.Line 248 still references
upstream_bib..fedora.fedora_40but the variant was renamed tofedora_legacy(line 51). Update to maintain consistency.Apply this diff:
- upstream_bib..centos, upstream_bib..local_image, upstream_bib..fedora.fedora_40: + upstream_bib..centos, upstream_bib..local_image, upstream_bib..fedora.fedora_legacy:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
provider/bootc_image_builder/bootc_image_build_utils.py(4 hunks)virttools/tests/cfg/bootc_image_builder/bootc_disk_image_build.cfg(9 hunks)virttools/tests/cfg/bootc_image_builder/bootc_disk_image_install.cfg(9 hunks)virttools/tests/src/bootc_image_builder/bootc_disk_image_build.py(1 hunks)virttools/tests/src/bootc_image_builder/bootc_disk_image_install.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- virttools/tests/src/bootc_image_builder/bootc_disk_image_install.py
- provider/bootc_image_builder/bootc_image_build_utils.py
- virttools/tests/src/bootc_image_builder/bootc_disk_image_build.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.9
- GitHub Check: Python 3.11
- GitHub Check: Python 3.12
- GitHub Check: Python 3.8
🔇 Additional comments (9)
virttools/tests/cfg/bootc_image_builder/bootc_disk_image_build.cfg (3)
198-207: LGTM: rhel_9.7_nightly_bib configuration block.The new bib entry for RHEL 9.7 nightly follows the established pattern from 9.5/9.6 nightly bibs with appropriate registry, credentials, and anaconda-iso mappings.
108-114: LGTM: rhel_9.7_nightly variant.The variant correctly mirrors the rhel_9.6_nightly structure with updated version ID and container URL.
121-126: LGTM: rhel_10.1_nightly variant.Configuration follows the rhel_10.0_nightly pattern with TLS verification disabled and anaconda-iso excluded.
virttools/tests/cfg/bootc_image_builder/bootc_disk_image_install.cfg (6)
236-247: Verify empty aws_config_dict pattern.Lines 236-247 set
aws_config_dict = {}for multiple bib/image combinations. Confirm this intentionally disables AWS AMI generation for these variants (rhel_9.4_bib, rhel_9.4_nightly_bib, and all nightly centos/local_image builds for 9.5/9.6/9.7/10.0/10.1).Is AWS upload intentionally disabled for all nightly bibs with centos/local_image? Document the rationale if this is a deliberate scope reduction.
190-199: LGTM: rhel_9.7_nightly_bib configuration block.Matches the pattern from rhel_9.6_nightly_bib with updated version-specific values.
116-120: LGTM: rhel_9.7_nightly variant.Configuration correctly mirrors rhel_9.6_nightly with TLS disabled and anaconda-iso excluded.
126-130: LGTM: rhel_10.1_nightly variant.Follows the rhel_10.0_nightly pattern with updated container URL and version ID.
89-92: LGTM: rhel_9.7_nightly_bib FIPS enablement.The local_image block correctly adds FIPS support for RHEL 9.7 nightly, mirroring 9.5/9.6 configuration.
100-101: Verify upstream_bib build_container override.Same concern as in build.cfg: line 100-101 assigns
rhel10/rhel-bootc:10.1from the stage registry to bothrhel_10.1_bibandupstream_bib. Confirmupstream_bibshould use stage registry images rather than public ones.See verification script in bootc_disk_image_build.cfg review.
chunfuwen
force-pushed
the
enable_bootc_on_rhel9.7_10.1
branch
4 times, most recently
from
c55b6f0 to
3192704
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
virttools/tests/cfg/bootc_image_builder/bootc_disk_image_build.cfg (4)
29-29: Update fedora_40 reference to fedora_legacy.This line still references
fedora_40despite the rename tofedora_legacyat line 54. Past review identified this same issue which was marked as addressed in commit c55b6f0, but the inconsistency remains.Apply this diff:
- anaconda-iso..upstream_bib..fedora_40: + anaconda-iso..upstream_bib..fedora_legacy:
90-91: Split upstream_bib from stage registry assignment.Lines 90-91 group
upstream_bibwithrhel_10.2_bib, causingupstream_bibto inherit the private stage registry imageregistry.stage.redhat.io/rhel10/rhel-bootc:10.2. This duplicates the issue previously flagged for therhel_10.1_bibgrouping. Theupstream_bibvariant should use publicly accessible images, not internal stage registry resources.Apply this diff to separate the assignments:
+ rhel_10.2_bib: + build_container = "registry.stage.redhat.io/rhel10/rhel-bootc:10.2" - rhel_10.2_bib, upstream_bib: - build_container = "registry.stage.redhat.io/rhel10/rhel-bootc:10.2" + upstream_bib: + build_container = "<publicly-accessible-image>"
245-254: Remove internal BOS compose URL.Line 254 contains a hard-coded internal BOS URL (
download.eng.bos.redhat.com) with a specific compose ID. Past review recommended replacing this with a CI-injected placeholder to avoid exposing internal infrastructure details and fixed build identifiers in the public repository.Apply this diff:
- compose_url = "https://download.eng.bos.redhat.com/rhel-10/nightly/RHEL-10-Public-Beta/RHEL-10.1-20250522.76" + compose_url = "LATEST_RHEL_10_1_COMPOSE_URL"
255-264: Remove internal BOS compose URL.Line 264 contains a hard-coded internal BOS URL with a specific compose ID. Replace with a CI-injected placeholder to avoid exposing internal infrastructure details.
Apply this diff:
- compose_url = "https://download.eng.bos.redhat.com/rhel-10/nightly/RHEL-10-Public-Beta/RHEL-10.2-20251022.76" + compose_url = "LATEST_RHEL_10_2_COMPOSE_URL"virttools/tests/cfg/bootc_image_builder/bootc_disk_image_install.cfg (4)
111-112: Split upstream_bib from stage registry assignment.Lines 111-112 group
upstream_bibwithrhel_10.2_bib, causingupstream_bibto inherit the private stage registry image. This mirrors the same issue inbootc_disk_image_build.cfgat lines 90-91. Theupstream_bibvariant should use a publicly accessible image rather than internal stage registry resources.Apply this diff:
+ rhel_10.2_bib: + build_container = "registry.stage.redhat.io/rhel10/rhel-bootc:10.2" - rhel_10.2_bib, upstream_bib: - build_container = "registry.stage.redhat.io/rhel10/rhel-bootc:10.2" + upstream_bib: + build_container = "<publicly-accessible-image>"
241-250: Remove internal BOS compose URL.Line 250 contains a hard-coded internal BOS URL with a specific compose ID. Past review recommended replacing this with a CI-injected placeholder. This should match the approach used in
bootc_disk_image_build.cfg.Apply this diff:
- compose_url = "https://download.eng.bos.redhat.com/rhel-10/nightly/RHEL-10-Public-Beta/RHEL-10.1-20250522.76" + compose_url = "LATEST_RHEL_10_1_COMPOSE_URL"
251-260: Remove internal BOS compose URL.Line 260 contains a hard-coded internal BOS URL with a specific compose ID. Replace with a CI-injected placeholder to avoid exposing internal infrastructure details.
Apply this diff:
- compose_url = "https://download.eng.bos.redhat.com/rhel-10/nightly/RHEL-10-Public-Beta/RHEL-10.2-20250522.76" + compose_url = "LATEST_RHEL_10_2_COMPOSE_URL"
293-293: Update fedora_40 reference to fedora_legacy.Line 293 still references
fedora_40in the AWS config dictionary mapping. This should befedora_legacyto match the rename at line 51. Past review identified this issue and marked it as addressed in commit c55b6f0, but the inconsistency remains.Apply this diff:
- upstream_bib..centos, upstream_bib..local_image, upstream_bib..fedora.fedora_40: + upstream_bib..centos, upstream_bib..local_image, upstream_bib..fedora.fedora_legacy:
🧹 Nitpick comments (1)
virttools/tests/src/bootc_image_builder/bootc_disk_image_install.py (1)
96-97: Consider reordering for better readability.The bib_ref list is functionally correct but the ordering mixes RHEL 9.8 after 10.1, making it harder to scan. Grouping RHEL 9.x entries together followed by RHEL 10.x entries would improve maintainability.
Apply this diff to improve ordering:
- if bib_ref in ["upstream_bib", "rhel_9.4_nightly_bib", "rhel_9.5_nightly_bib", - "rhel_9.6_nightly_bib", "rhel_9.7_nightly_bib", "rhel_10.0_bib", "rhel_10.1_bib", "rhel_9.8_nightly_bib", "rhel_10.2_bib"]: + if bib_ref in ["upstream_bib", "rhel_9.4_nightly_bib", "rhel_9.5_nightly_bib", + "rhel_9.6_nightly_bib", "rhel_9.7_nightly_bib", "rhel_9.8_nightly_bib", + "rhel_10.0_bib", "rhel_10.1_bib", "rhel_10.2_bib"]:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
provider/bootc_image_builder/bootc_image_build_utils.py(5 hunks)virttools/tests/cfg/bootc_image_builder/bootc_disk_image_build.cfg(9 hunks)virttools/tests/cfg/bootc_image_builder/bootc_disk_image_install.cfg(9 hunks)virttools/tests/src/bootc_image_builder/bootc_disk_image_build.py(1 hunks)virttools/tests/src/bootc_image_builder/bootc_disk_image_install.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- provider/bootc_image_builder/bootc_image_build_utils.py
- virttools/tests/src/bootc_image_builder/bootc_disk_image_build.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.12
- GitHub Check: Python 3.11
- GitHub Check: Python 3.9
- GitHub Check: Python 3.8
🔇 Additional comments (10)
virttools/tests/cfg/bootc_image_builder/bootc_disk_image_build.cfg (4)
112-125: LGTM! RHEL 9.7 and 9.8 nightly variant additions are well-structured.The new
rhel_9.7_nightlyandrhel_9.8_nightlyvariant blocks follow the established pattern for nightly builds, correctly specifying stage registry URLs, version IDs, and LVM partition settings for qcow images.
132-143: LGTM! RHEL 10.1 and 10.2 nightly variant additions are consistent.The new
rhel_10.1_nightlyandrhel_10.2_nightlyvariants correctly mirror the RHEL 10.0 nightly pattern, with appropriate stage registry URLs, TLS verification disabled, and version IDs set.
215-234: LGTM! New bib metadata blocks are properly structured.The
rhel_9.7_nightly_bibandrhel_9.8_nightly_bibmetadata blocks correctly follow the established pattern, including registry keys, stage credentials, bib_image_url references, and anaconda-iso configuration with placeholder compose URLs.
301-307: LGTM! VHD and GCE disk type restrictions updated appropriately.The
onlystatements forvhdandgcedisk image types now reference the latest RHEL versions (9.8 and 10.2 nightly bibs), appropriately focusing the test matrix on current releases.virttools/tests/cfg/bootc_image_builder/bootc_disk_image_install.cfg (6)
43-71: LGTM! Fedora rename and version updates applied correctly.The
fedora_legacyrename (line 51) is correctly applied in this file, and the Fedora bootc container version for s390-virtio is appropriately updated to 42. The centos10 variant restrictions properly include the latest RHEL test bibs.
89-106: LGTM! RHEL 9.7 and 9.8 FIPS and anaconda-iso configurations are consistent.The FIPS enablement for
rhel_9.7_nightly_bibandrhel_9.8_nightly_bib, along with the specialized anaconda-iso configurations (including toml config and kickstart settings), correctly follow the established pattern for RHEL 9.x nightly builds.
127-151: LGTM! New nightly variant blocks are properly configured.The
rhel_9.7_nightly,rhel_9.8_nightly,rhel_10.1_nightly, andrhel_10.2_nightlyvariant blocks correctly specify stage registry URLs, TLS settings, version IDs, and appropriately exclude anaconda-iso testing.
211-230: LGTM! New bib metadata blocks follow established patterns.The
rhel_9.7_nightly_bibandrhel_9.8_nightly_bibmetadata correctly include registry keys, credentials, bib_image_url references, and use appropriate placeholder compose URLs.
277-292: LGTM! AWS config dictionary assignments properly extended.The
aws_config_dict = {}assignments for new RHEL 9.7, 9.8, 10.1, and 10.2 bib variants correctly limit AWS provisioning to specific test combinations, maintaining selective test coverage.
323-329: LGTM! VHD and GCE install test restrictions updated consistently.The
onlystatements forvhdandgcedisk image install tests now reference the latest RHEL versions, matching the corresponding changes inbootc_disk_image_build.cfgand appropriately focusing the test matrix.
chunfuwen
force-pushed
the
enable_bootc_on_rhel9.7_10.1
branch
from
3192704 to
090576e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
provider/bootc_image_builder/bootc_image_build_utils.py (1)
1344-1344: Document the registry image source change.The registry image was changed from
registry:2toghcr.io/osbuild/bootc-image-builder/registry:2. A past review comment (from smitterl) asked why this change was necessary.Please document the reason for switching to the GitHub Container Registry (ghcr.io) image—for example, whether it's for reliability, version pinning, or organizational requirements.
virttools/tests/cfg/bootc_image_builder/bootc_disk_image_build.cfg (3)
215-264: Replace internal BOS URLs with CI-injected placeholders.Lines 224, 254, and 264 contain hard-coded internal Red Hat compose URLs (e.g.,
https://download.eng.bos.redhat.com/rhel-10/nightly/RHEL-10-Public-Beta/RHEL-10.1-20250522.76). These internal URLs and specific compose IDs should not be committed to the repository.Replace these with environment variable placeholders (e.g.,
LATEST_RHEL_10_1_COMPOSE_URL) that will be injected by CI at runtime.
29-29: Inconsistent fedora variant name.Past review comments indicate that
fedora_40was renamed tofedora_legacy(line 54), but line 29 still references the old nameanaconda-iso..upstream_bib..fedora_40.Update to
anaconda-iso..upstream_bib..fedora_legacyfor consistency.
90-91:upstream_bibshould not inherit private stage registry image.Lines 90-91 group
upstream_bibwithrhel_10.2_bib, causingupstream_bibto inherit the private stage registry imageregistry.stage.redhat.io/rhel10/rhel-bootc:10.2. Theupstream_bibvariant is intended for public/upstream use and should use a publicly accessible image.Split
upstream_bibinto a separate assignment with a public image source, or document why the stage registry is appropriate for upstream builds.virttools/tests/cfg/bootc_image_builder/bootc_disk_image_install.cfg (2)
211-260: Replace internal BOS URLs with CI-injected placeholders.Lines 220, 230, 250, and 260 contain hard-coded internal Red Hat compose URLs (e.g.,
https://download.eng.bos.redhat.com/rhel-10/nightly/...). These internal URLs should not be committed to the repository.Replace with environment variable placeholders that will be injected by CI at runtime.
293-293: Inconsistent fedora variant reference.Line 293 still references
upstream_bib..fedora.fedora_40, but the variant was renamed tofedora_legacyelsewhere in the config. This was flagged in a past review and marked as addressed in commit c55b6f0, but the issue persists.Update to
upstream_bib..fedora.fedora_legacyfor consistency.
🧹 Nitpick comments (2)
virttools/tests/src/bootc_image_builder/bootc_disk_image_build.py (2)
84-85: Consider refactoring the duplicated bib_ref eligibility list.The same bib_ref list appears in both
bootc_disk_image_build.py(lines 84-85) andbootc_disk_image_install.py(lines 96-97). This duplication creates a maintenance burden—if a new bib variant is added in the future, developers must remember to update both locations.Consider extracting this list into a shared constant or utility function to ensure consistency and reduce maintenance overhead.
84-85: Inconsistent ordering in bib_ref list.The bib_ref entries are not sorted by version number. The current order mixes RHEL 9.x and 10.x variants unpredictably (e.g.,
rhel_9.7_nightly_bib,rhel_10.0_bib,rhel_10.1_bib,rhel_9.8_nightly_bib,rhel_10.2_bib).For better readability and maintainability, consider sorting the list by version (e.g., all RHEL 9.x entries first in ascending order, then all RHEL 10.x entries).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
provider/bootc_image_builder/bootc_image_build_utils.py(5 hunks)virttools/tests/cfg/bootc_image_builder/bootc_disk_image_build.cfg(9 hunks)virttools/tests/cfg/bootc_image_builder/bootc_disk_image_install.cfg(9 hunks)virttools/tests/src/bootc_image_builder/bootc_disk_image_build.py(1 hunks)virttools/tests/src/bootc_image_builder/bootc_disk_image_install.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.9
- GitHub Check: Python 3.8
- GitHub Check: Python 3.12
- GitHub Check: Python 3.11
🔇 Additional comments (11)
virttools/tests/src/bootc_image_builder/bootc_disk_image_install.py (1)
96-97: LGTM: Authentication list extended consistently.The bib_ref authentication list matches the corresponding list in
bootc_disk_image_build.py, ensuring consistent authentication handling across build and install workflows for the new RHEL 9.7/9.8 and 10.1/10.2 variants.provider/bootc_image_builder/bootc_image_build_utils.py (4)
67-67: LGTM: Quieter podman output improves logs.Adding
--quietand--log-level=infomakes podman output less verbose while retaining important information, which should improve log readability during image builds.
88-91: Global environment modification may have unintended side effects.Setting
os.environ["PODMAN_LOG_LEVEL"] = "error"on line 90 modifies the global environment and will affect all subsequent podman operations in the same process, not just the current build. If multiple builds run sequentially or if other podman commands execute after this point, they will all inherit the "error" log level.Consider whether this global side effect is intentional, or if the environment variable should be passed only to the specific podman command (e.g., via the
envparameter ofprocess.run()at line 126-127).
600-614: LGTM: Repo recognition extended for new RHEL versions.The pattern for recognizing RHEL 9.7, 9.8, 10.1, and 10.2 repositories is consistent with existing logic for earlier versions. This properly supports the custom repo handling needed for the new bib variants.
662-662: LGTM: Quiet build reduces output noise.Adding
--quietto the build command reduces unnecessary output during container builds, aligning with the quieter logging approach applied to podman commands elsewhere.virttools/tests/cfg/bootc_image_builder/bootc_disk_image_build.cfg (2)
112-143: LGTM: New nightly variant blocks follow established pattern.The new variant blocks for
rhel_9.7_nightly,rhel_9.8_nightly,rhel_10.1_nightly, andrhel_10.2_nightlyfollow the same structure as existing nightly variants, with appropriate container URLs, version IDs, andno anaconda-isorestrictions.
301-301: LGTM: VHD variant coverage updated.The
onlyfilter correctly restricts VHD builds toupstream_bib,rhel_9.8_nightly_bib, andrhel_10.2_bib, aligning with the new bib variants introduced in this PR.virttools/tests/cfg/bootc_image_builder/bootc_disk_image_install.cfg (4)
43-43: LGTM: Variant filter updated for new bibs.The
onlyfilter correctly includes the newrhel_10.2_bibandrhel_9.8_nightly_bibvariants alongsideupstream_bib, ensuring proper test coverage for CentOS 10 builds.
89-112: LGTM: Build container assignments consistent with build config.The build container assignments for the new RHEL 9.7, 9.8, 10.1, and 10.2 variants match those in
bootc_disk_image_build.cfg, ensuring consistency between build and install test configurations. The FIPS and anaconda-iso customizations are appropriately configured.
127-151: LGTM: New nightly variant blocks properly configured.The nightly variant blocks for RHEL 9.7, 9.8, 10.1, and 10.2 follow the established pattern with appropriate container URLs, TLS settings, bib restrictions, and anaconda-iso exclusions.
277-292: LGTM: AWS config dict assignments follow established pattern.The AWS config dict assignments for the new bib variants correctly set empty dictionaries for local_image and centos combinations, consistent with the pattern used for existing bib variants.
chunfuwen
force-pushed
the
enable_bootc_on_rhel9.7_10.1
branch
from
539b024 to
fcbeb2a
Compare
chunfuwen
changed the title
chunfuwen
force-pushed
the
enable_bootc_on_rhel9.7_10.1
branch
from
fcbeb2a to
7b8962e
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
virttools/tests/cfg/bootc_image_builder/bootc_disk_image_install.cfg (1)
293-294:⚠️ Potential issue | 🟡 MinorUpdate references from
fedora_40tofedora_legacyin two configuration files.The variant was renamed to
fedora_legacybut two references still use the old name:
- Line 293 in
bootc_disk_image_install.cfg:upstream_bib..fedora.fedora_40- Line 29 in
bootc_disk_image_build.cfg:anaconda-iso..upstream_bib..fedora_40Both should reference
fedora_legacyinstead.🐛 Proposed fixes
In
bootc_disk_image_install.cfg:- upstream_bib..centos, upstream_bib..local_image, upstream_bib..fedora.fedora_40: + upstream_bib..centos, upstream_bib..local_image, upstream_bib..fedora.fedora_legacy:In
bootc_disk_image_build.cfg:- anaconda-iso..upstream_bib..fedora_40: + anaconda-iso..upstream_bib..fedora_legacy:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@virttools/tests/cfg/bootc_image_builder/bootc_disk_image_install.cfg` around lines 293 - 294, Replace the two stale variant references that still use "fedora_40" with "fedora_legacy": change the key upstream_bib..fedora.fedora_40 to upstream_bib..fedora_legacy in the bootc_disk_image_install.cfg entry and change anaconda-iso..upstream_bib..fedora_40 to anaconda-iso..upstream_bib..fedora_legacy in the build cfg entry so both config entries reference the renamed variant consistently.
🧹 Nitpick comments (1)
provider/bootc_image_builder/bootc_image_build_utils.py (1)
88-91: Global environment modification may persist beyond this build.Setting
os.environ["PODMAN_LOG_LEVEL"]modifies the process-wide environment. Ifpodman_command_buildis called multiple times in the same process (e.g., for different disk_image_types), the environment variable remains set to "error" even for non-anaconda-iso builds.Consider either:
- Passing the log level via the command line only (already done with
--log-level=error)- Restoring the original value after the command completes
🔧 Proposed fix to avoid persistent environment modification
if disk_image_type in ['anaconda-iso']: cmd += " --use-librepo " - os.environ["PODMAN_LOG_LEVEL"] = "error" cmd = cmd.replace("--log-level=info", "--log-level=error")If
PODMAN_LOG_LEVELis truly needed for the subprocess, consider using theenvparameter ofprocess.run()instead of modifying the global environment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@provider/bootc_image_builder/bootc_image_build_utils.py` around lines 88 - 91, The code sets os.environ["PODMAN_LOG_LEVEL"] inside podman_command_build (when disk_image_type == 'anaconda-iso'), which mutates process-wide environment and can leak to subsequent builds; change this to avoid global mutation by either removing the env assignment and relying on the existing cmd replacement (--log-level=error), or if an env var is required for the subprocess, pass a copy of os.environ with PODMAN_LOG_LEVEL set via the env argument to the subprocess call (or restore the original os.environ value after the call) instead of writing directly to os.environ.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@virttools/tests/cfg/bootc_image_builder/bootc_disk_image_install.cfg`:
- Around line 293-294: Replace the two stale variant references that still use
"fedora_40" with "fedora_legacy": change the key upstream_bib..fedora.fedora_40
to upstream_bib..fedora_legacy in the bootc_disk_image_install.cfg entry and
change anaconda-iso..upstream_bib..fedora_40 to
anaconda-iso..upstream_bib..fedora_legacy in the build cfg entry so both config
entries reference the renamed variant consistently.
---
Nitpick comments:
In `@provider/bootc_image_builder/bootc_image_build_utils.py`:
- Around line 88-91: The code sets os.environ["PODMAN_LOG_LEVEL"] inside
podman_command_build (when disk_image_type == 'anaconda-iso'), which mutates
process-wide environment and can leak to subsequent builds; change this to avoid
global mutation by either removing the env assignment and relying on the
existing cmd replacement (--log-level=error), or if an env var is required for
the subprocess, pass a copy of os.environ with PODMAN_LOG_LEVEL set via the env
argument to the subprocess call (or restore the original os.environ value after
the call) instead of writing directly to os.environ.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dd34671d-58a6-40a8-bc2c-1ea170e12e97
📒 Files selected for processing (5)
provider/bootc_image_builder/bootc_image_build_utils.pyvirttools/tests/cfg/bootc_image_builder/bootc_disk_image_build.cfgvirttools/tests/cfg/bootc_image_builder/bootc_disk_image_install.cfgvirttools/tests/src/bootc_image_builder/bootc_disk_image_build.pyvirttools/tests/src/bootc_image_builder/bootc_disk_image_install.py
🚧 Files skipped from review as they are similar to previous changes (1)
- virttools/tests/cfg/bootc_image_builder/bootc_disk_image_build.cfg
smitterl
left a comment
smitterl
left a comment
There was a problem hiding this comment.
Thank you for this @chunfuwen It's been a while since I last reviewed this but I think generally your commit message should contain some of the details that you explained in comments. The rest LGTM.
Disable unnecessary tests on old rhel release as per test strategy Remove hardcode compose_url update registry container image from the more reliable source:ghcr.io/osbuild/bootc-image-builder is more reliable registry Remove unactive distribution testing such as REHL 9.5/9.6/10.0 Add qcow2 image case to cover newly introduced feature :lvm disk partition Signed-off-by: chunfuwen <chwen@redhat.com>
chunfuwen
force-pushed
the
enable_bootc_on_rhel9.7_10.1
branch
from
7b8962e to
5ca58ba
Compare
|
@smitterl adding your replied comments into commit message now |
2 participants
Uh oh!
There was an error while loading. Please reload this page.