WIP: [ART-14277] add support for coreos-10 #2243
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Excluded labels (none allowed) (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR enhances RHCOS handling to support per-container and per-RHEL version processing. Changes include reworking RHCOS status detection to iterate over multiple primary containers, adding per-container helper methods, updating method signatures to accept primary_container parameters, extending JSON schema definitions, and implementing per-RHEL version RHCOS build triggering. Changes
Sequence DiagramsequenceDiagram
participant Scanner as Scan Sources<br/>(scan_konflux)
participant Finder as RHCOS Build<br/>Finder
participant Inspector as RHCOS Build<br/>Inspector
participant Pipeline as Build RHCOS<br/>Pipeline
rect rgb(240, 248, 255)
note over Scanner,Pipeline: New Per-Container Processing Flow
end
loop For each primary_container in containers
Scanner->>Finder: RHCOSBuildFinder(...,<br/>primary_container=X)
Finder->>Finder: Store primary_container
Scanner->>Scanner: Determine rhel_version<br/>from payload tags
Scanner->>Scanner: check_rhcos_changes(<br/>primary_container,<br/>rhel_version)
alt RHCOS changes detected
Scanner->>Scanner: Record rhel_version
end
Scanner->>Scanner: check_outdated_rpms(<br/>primary_container,<br/>rhel_version)
alt Outdated RPMs found
Scanner->>Scanner: Record rhel_version
end
end
rect rgb(255, 250, 205)
note over Scanner,Pipeline: Per-RHEL Version Build Triggering
end
loop For each outdated rhel_version
Scanner->>Pipeline: Trigger build with<br/>version="{base}-{rhel_version}"
Pipeline->>Pipeline: Parse and set<br/>rhel_version attribute
Pipeline->>Finder: Construct stream using<br/>rhel_version
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
Ximinhan
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
doozer/doozerlib/rhcos.py (1)
223-223: Fix logic error: useandinstead oforin conditional.The condition
if container_conf or container_conf != primary_conf:short-circuits on the first truthycontainer_conf, so the comparisoncontainer_conf != primary_confis never evaluated. The intent appears to be checking whethercontainer_confis provided AND differs fromprimary_conf.Apply this diff:
- if container_conf or container_conf != primary_conf: + if container_conf and container_conf != primary_conf: rhcosdata = util.oc_image_info_for_arch(container_conf.rhcos_index_tag, self.go_arch)
🧹 Nitpick comments (3)
pyartcd/pyartcd/pipelines/build_rhcos.py (1)
41-43: Version/rhel_version parsing looks correct; consider simplifying split logic.The new handling:
- Uses the base OCP version (
self.version = version.split('-')[0]) for group lookup.- Uses
self.rhel_versionwhen provided to override default4.x-EL.y/rhel-EL.ystream names for layered RHCOS jobs.That matches the new
build_version="<ocp>-<rhel>"convention.To reduce repeated splitting and better handle unexpected extra
-segments, consider:- self.version = version.split('-')[0] + version_parts = version.split('-', 1) + self.version = version_parts[0] self.job = job - self.rhel_version = version.split('-')[1] if len(version.split('-')) > 1 else None + self.rhel_version = version_parts[1] if len(version_parts) > 1 else NoneThis keeps behavior the same for inputs like
4.16-10.4but avoids multiple splits and makes it clearer how additional-characters are treated.Also applies to: 119-125
doozer/doozerlib/cli/scan_sources_konflux.py (1)
1443-1455: Guard latest_rhcos_node_shasum against missing rhcos_index_tag for a container.
latest_rhcos_node_shasum()now derives the index image/tag fromgroup_config.rhcos.payload_tagsby matchingtag.name == primary_container:rhcos_index = next( ( tag.rhcos_index_tag for tag in self.runtime.group_config.rhcos.payload_tags if tag.name == primary_container ), "", ) rhcos_info = util.oc_image_info_for_arch(rhcos_index, go_arch)If there’s no matching entry or
rhcos_index_tagis missing,rhcos_indexbecomes an empty string andoc_image_info_for_archis likely to fail in a non-obvious way.To make this more robust and easier to debug:
- rhcos_index = next( - ( - tag.rhcos_index_tag - for tag in self.runtime.group_config.rhcos.payload_tags - if tag.name == primary_container - ), - "", - ) - rhcos_info = util.oc_image_info_for_arch(rhcos_index, go_arch) - return rhcos_info['digest'] + rhcos_index = next( + ( + tag.rhcos_index_tag + for tag in self.runtime.group_config.rhcos.payload_tags + if tag.name == primary_container + ), + None, + ) + if not rhcos_index: + self.logger.warning( + "No rhcos_index_tag configured for primary_container %s; skipping latest node lookup", + primary_container, + ) + return None + + rhcos_info = util.oc_image_info_for_arch(rhcos_index, go_arch) + return rhcos_info['digest']This keeps existing behavior when configuration is correct while failing gracefully (and noisily) when it is not.
doozer/doozerlib/rhcos.py (1)
31-39: Use explicitOptionaltype hint forprimary_container.Per PEP 484, parameters with
Nonedefaults should use explicitOptional[str]orstr | Nonetype hints rather than implicitOptional.Apply this diff:
def __init__( self, runtime, version: str, brew_arch: str = "x86_64", private: bool = False, custom: bool = False, - primary_container: str = None, + primary_container: Optional[str] = None, ):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (6)
doozer/doozerlib/cli/inspect_stream.py(1 hunks)doozer/doozerlib/cli/scan_sources_konflux.py(2 hunks)doozer/doozerlib/rhcos.py(5 hunks)ocp-build-data-validator/validator/json_schemas/rhcos.schema.json(1 hunks)pyartcd/pyartcd/pipelines/build_rhcos.py(2 hunks)pyartcd/pyartcd/pipelines/ocp4_scan_konflux.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pyartcd/pyartcd/pipelines/ocp4_scan_konflux.py (1)
pyartcd/pyartcd/jenkins.py (1)
start_rhcos(431-436)
🪛 Ruff (0.14.5)
doozer/doozerlib/cli/scan_sources_konflux.py
1393-1393: Undefined name version
(F821)
1411-1411: Undefined name status
(F821)
1412-1412: Undefined name status
(F821)
1413-1413: Undefined name status
(F821)
1414-1414: Undefined name status
(F821)
1418-1418: Undefined name status
(F821)
doozer/doozerlib/rhcos.py
38-38: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
⏰ 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). (1)
- GitHub Check: tests
🔇 Additional comments (3)
doozer/doozerlib/cli/inspect_stream.py (1)
38-38: Non-functional TODO only; no issues.The added TODO comment has no runtime effect; existing
INCONSISTENT_RHCOS_RPMShandling remains unchanged.ocp-build-data-validator/validator/json_schemas/rhcos.schema.json (1)
82-99: RHCOS-10 schema extensions are consistent with existing variants.The new
rhel-coreos-10*andrhel-coreos-10-extensions*properties mirror the 8/“base” variants and correctly reusemachine-os-contentvia$ref.pyartcd/pyartcd/pipelines/ocp4_scan_konflux.py (1)
33-33: Clarify Issue 1 is incorrect due to util.get_changes() transformation; Issue 2 remains valid.After examining the data flow, Issue 1 in the original review is incorrect:
The transformation in
util.get_changes()at line 259 explicitly wraps the entire original rhcos object under the'reason'key:rhcos = [{'name': rhcos['name'], 'reason': rhcos} for rhcos in yaml_data.get('rhcos', [])]Since doozer's
check_rhcos_changes()andcheck_outdated_rpms()set'updated','outdated', and'rhel_version'as top-level keys on the status dict, they become accessible asrhcos_change['reason'].get('updated'), etc., after the wrapping. The code accessingrhcos_change['reason'].get('updated')will work correctly and will not raise anAttributeError.However, Issue 2 remains valid: when
self.rhcos_inconsistentis True butself.rhcos_outdatedis False,self.rhcos_outdated_rhel_versionremains empty, so the loop at lines 189–191 iterates over an empty set and never triggers astart_rhcoscall—contrary to the previous behavior. This should be fixed to preserve the fallback trigger for inconsistency-only cases.Additionally, lines 118–120 should filter out
Nonevalues fromself.rhcos_outdated_rhel_versionto avoid"4.16-None"build_version strings.Likely an incorrect or invalid review comment.
| primary_containers = get_primary_container_name(self.runtime) | ||
| for arch in self.runtime.arches: | ||
| brew_arch = brew_arch_for_go_arch(arch) | ||
| for private in (False, True): | ||
| status = dict(name=f"{version}-{brew_arch}{'-priv' if private else ''}") | ||
| if self.runtime.group_config.rhcos.get("layered_rhcos", False): | ||
| tagged_rhcos_value = self.tagged_rhcos_node_digest(primary_container, version, brew_arch, private) | ||
| latest_rhcos_value = self.latest_rhcos_node_shasum(arch) | ||
| else: | ||
| tagged_rhcos_value = self.tagged_rhcos_id(primary_container, version, brew_arch, private) | ||
| latest_rhcos_value = self.latest_rhcos_build_id(version, brew_arch, private) | ||
|
|
||
| if latest_rhcos_value and tagged_rhcos_value != latest_rhcos_value: | ||
| status['updated'] = True | ||
| status['changed'] = True | ||
| status['reason'] = ( | ||
| f"latest RHCOS build is {latest_rhcos_value} which differs from istag {tagged_rhcos_value}" | ||
| ) | ||
| statuses.append(status) | ||
| # check outdate rpms in rhcos | ||
| pullspec_for_tag = dict() | ||
| build_id = "" | ||
| for container_conf in self.runtime.group_config.rhcos.payload_tags: | ||
| build_id, pullspec = rhcos.RHCOSBuildFinder( | ||
| self.runtime, version, brew_arch, private | ||
| ).latest_container(container_conf) | ||
| pullspec_for_tag[container_conf.name] = pullspec | ||
| non_latest_rpms = await rhcos.RHCOSBuildInspector( | ||
| self.runtime, pullspec_for_tag, brew_arch, build_id | ||
| ).find_non_latest_rpms(exclude_rhel=True) | ||
| non_latest_rpms_filtered = [] | ||
|
|
||
| # exclude rpm if non_latest_rpms in rhel image rpm list | ||
| exclude_rpms = self.runtime.group_config.rhcos.get("exempt_rpms", []) | ||
| for installed_rpm, latest_rpm, repo in non_latest_rpms: | ||
| if any(excluded in installed_rpm for excluded in exclude_rpms): | ||
| self.logger.info( | ||
| f"[EXEMPT SKIPPED] Exclude {installed_rpm} because its in the exempt list when {latest_rpm} was available in repo {repo}" | ||
| ) | ||
| else: | ||
| non_latest_rpms_filtered.append((installed_rpm, latest_rpm, repo)) | ||
| if non_latest_rpms_filtered: | ||
| status['outdated'] = True | ||
| status['changed'] = True | ||
| status['reason'] = ";\n".join( | ||
| f"Outdated RPM {installed_rpm} installed in RHCOS ({brew_arch}) when {latest_rpm} was available in repo {repo}" | ||
| for installed_rpm, latest_rpm, repo in non_latest_rpms_filtered | ||
| for primary_container in primary_containers: | ||
| rhel_version = self.runtime.group_config.rhcos.payload_tags.get(primary_container, {}).get( | ||
| "rhel_version", None | ||
| ) | ||
| statuses.append(status) | ||
|
|
||
| if not rhel_version: | ||
| rhel_version = f"{self.runtime.group_config.vars.RHCOS_EL_MAJOR}.{self.runtime.group_config.vars.RHCOS_EL_MINOR}" | ||
| status = self.check_rhcos_changes(primary_container, brew_arch, private, rhel_version) | ||
| if status: | ||
| statuses.append(status) | ||
| status = self.check_outdated_rpms(primary_container, brew_arch, private, rhel_version) | ||
| if status: | ||
| statuses.append(status) | ||
| self.rhcos_status = statuses |
There was a problem hiding this comment.
🧩 Analysis chain
Fix async usage and undefined names in per-container RHCOS RPM checks.
There are a few hard correctness issues here:
awaitin non-async function
check_outdated_rpms() is declared as a regular method:
def check_outdated_rpms(self, primary_container, brew_arch, private, rhel_version):
...
non_latest_rpms = await rhcos.RHCOSBuildInspector(...).find_non_latest_rpms(...)Using await here is a syntax error; the file won’t even import. It also means detect_rhcos_status() calling self.check_outdated_rpms(...) is not actually awaiting the async work.
- Undefined
versionandstatus
Within check_outdated_rpms():
versionis referenced but never defined in the scope.statusis used (status['outdated'] = True, etc.) but never initialized, so even once the syntax issue is fixed this would raiseNameError.
You likely want semantics parallel to check_rhcos_changes() (same name field, plus outdated/rhel_version and reason).
- Suggested concrete fix
Make check_outdated_rpms() async, initialize version and status, and await it from detect_rhcos_status():
@@
- statuses = []
- primary_containers = get_primary_container_name(self.runtime)
+ statuses = []
+ primary_containers = get_primary_container_name(self.runtime)
@@
- for private in (False, True):
- for primary_container in primary_containers:
- rhel_version = self.runtime.group_config.rhcos.payload_tags.get(primary_container, {}).get(
- "rhel_version", None
- )
- if not rhel_version:
- rhel_version = f"{self.runtime.group_config.vars.RHCOS_EL_MAJOR}.{self.runtime.group_config.vars.RHCOS_EL_MINOR}"
- status = self.check_rhcos_changes(primary_container, brew_arch, private, rhel_version)
- if status:
- statuses.append(status)
- status = self.check_outdated_rpms(primary_container, brew_arch, private, rhel_version)
- if status:
- statuses.append(status)
+ for private in (False, True):
+ for primary_container in primary_containers:
+ rhel_version = self.runtime.group_config.rhcos.payload_tags.get(primary_container, {}).get(
+ "rhel_version", None
+ )
+ if not rhel_version:
+ rhel_version = (
+ f"{self.runtime.group_config.vars.RHCOS_EL_MAJOR}."
+ f"{self.runtime.group_config.vars.RHCOS_EL_MINOR}"
+ )
+ status = self.check_rhcos_changes(primary_container, brew_arch, private, rhel_version)
+ if status:
+ statuses.append(status)
+ status = await self.check_outdated_rpms(primary_container, brew_arch, private, rhel_version)
+ if status:
+ statuses.append(status)
@@
- def check_outdated_rpms(self, primary_container, brew_arch, private, rhel_version):
- # check outdate rpms in rhcos
- pullspec_for_tag = dict()
- build_id = ""
- for container_conf in self.runtime.group_config.rhcos.payload_tags:
- build_id, pullspec = rhcos.RHCOSBuildFinder(
- self.runtime, version, brew_arch, private, primary_container=primary_container
- ).latest_container(container_conf)
- pullspec_for_tag[container_conf.name] = pullspec
- non_latest_rpms = await rhcos.RHCOSBuildInspector(
- self.runtime, pullspec_for_tag, brew_arch, build_id, primary_container=primary_container
- ).find_non_latest_rpms(exclude_rhel=True)
+ async def check_outdated_rpms(self, primary_container, brew_arch, private, rhel_version):
+ # check outdated rpms in rhcos
+ version = self.runtime.get_minor_version()
+ status = dict(name=f"{version}-{brew_arch}{'-priv' if private else ''}")
+
+ pullspec_for_tag: Dict[str, str] = {}
+ build_id = ""
+ for container_conf in self.runtime.group_config.rhcos.payload_tags:
+ build_id, pullspec = rhcos.RHCOSBuildFinder(
+ self.runtime,
+ version,
+ brew_arch,
+ private,
+ primary_container=primary_container,
+ ).latest_container(container_conf)
+ pullspec_for_tag[container_conf.name] = pullspec
+
+ non_latest_rpms = await rhcos.RHCOSBuildInspector(
+ self.runtime,
+ pullspec_for_tag,
+ brew_arch,
+ build_id,
+ primary_container=primary_container,
+ ).find_non_latest_rpms(exclude_rhel=True)
@@
- if non_latest_rpms_filtered:
- status['outdated'] = True
- status['changed'] = True
- status['rhel_version'] = rhel_version
- status['reason'] = ";\n".join(
+ if non_latest_rpms_filtered:
+ status['outdated'] = True
+ status['changed'] = True
+ status['rhel_version'] = rhel_version
+ status['reason'] = ";\n".join(
f"Outdated RPM {installed_rpm} installed in RHCOS {primary_container} ({brew_arch}) when {latest_rpm} was available in repo {repo}"
for installed_rpm, latest_rpm, repo in non_latest_rpms_filtered
)
return statusThis addresses:
- Syntax error from
awaitin a non-async function. - F821 undefined
versionandstatus. - Status structure alignment with
check_rhcos_changes()and the report output.
- Type of
payload_tags
Note that in latest_rhcos_node_shasum() you iterate self.runtime.group_config.rhcos.payload_tags as a sequence of objects with .name and .rhcos_index_tag, but here you treat payload_tags as a mapping (.get(primary_container, {})). Please confirm the actual type; if it’s a list of models, you probably want a lookup similar to latest_rhcos_node_shasum() when deriving rhel_version.
A quick rg -n "payload_tags" doozer across the repo can confirm the expected structure (list vs mapping) and help ensure that rhel_version lookup and iteration are consistent everywhere.
Also applies to: 1387-1419
🏁 Script executed:
#!/bin/bash
# Examine the file around the mentioned lines to confirm issues
echo "=== Lines 1346-1362 (detect_rhcos_status loop) ==="
sed -n '1346,1362p' doozer/doozerlib/cli/scan_sources_konflux.py
echo ""
echo "=== Lines 1387-1419 (check_outdated_rpms method) ==="
sed -n '1387,1419p' doozer/doozerlib/cli/scan_sources_konflux.py
echo ""
echo "=== Lines 1443-1455 (latest_rhcos_node_shasum method) ==="
sed -n '1443,1455p' doozer/doozerlib/cli/scan_sources_konflux.py
echo ""
echo "=== Check detect_rhcos_status method signature ==="
sed -n '1340,1365p' doozer/doozerlib/cli/scan_sources_konflux.py
echo ""
echo "=== Check check_rhcos_changes method signature ==="
rg -n "def check_rhcos_changes" doozer/doozerlib/cli/scan_sources_konflux.py -A 3Length of output: 5429
🏁 Script executed:
#!/bin/bash
# Search for payload_tags usages to confirm structure
echo "=== All payload_tags usages in scan_sources_konflux.py ==="
rg -n "payload_tags" doozer/doozerlib/cli/scan_sources_konflux.py -B2 -A2
echo ""
echo "=== Check detect_rhcos_status method signature (async?) ==="
sed -n '1330,1345p' doozer/doozerlib/cli/scan_sources_konflux.py
echo ""
echo "=== Search for similar rhel_version derivation patterns ==="
rg -n "rhel_version" doozer/doozerlib/cli/scan_sources_konflux.py -B1 -A1Length of output: 3065
Fix async declaration, initialize undefined variables, and correct payload_tags type mismatch.
The review comment is correct. There are critical issues:
-
awaitin non-async function (line 1387–1396)
check_outdated_rpms()must be declaredasyncto useawait. -
Undefined
versionandstatusvariables (lines 1393, 1411–1414)
versionis never defined;statusis never initialized. Follow the pattern fromcheck_rhcos_changes()(line 1366–1367): initialize both. -
payload_tags type mismatch (line 1351 vs. lines 1391, 1449)
- Line 1351 treats it as a dict:
.get(primary_container, {}) - Lines 1391 and 1449 treat it as a sequence of objects with
.nameattribute
The latter is correct. Line 1351's dict lookup will fail at runtime. You need to derive
rhel_versionby searching the sequence, similar tolatest_rhcos_node_shasum()logic (lines 1449–1451). - Line 1351 treats it as a dict:
-
Missing
awaitin caller (line 1359)
Oncecheck_outdated_rpms()isasync, line 1359 must bestatus = await self.check_outdated_rpms(...).
Suggested fixes:
- Change line 1387 to
async def check_outdated_rpms(...) - Add at line 1388–1389:
version = self.runtime.get_minor_version()andstatus = dict(name=f"...") - Fix line 1351–1352 to iterate and match like
latest_rhcos_node_shasum(), or extractrhel_versionfrom the container_conf object if available - Change line 1359 to
status = await self.check_outdated_rpms(...)
🤖 Prompt for AI Agents
In doozer/doozerlib/cli/scan_sources_konflux.py around lines 1346–1362 and the
related check_outdated_rpms block (roughly lines 1387–1414), make
check_outdated_rpms an async function, initialize the local variables used there
(define version from runtime, e.g. using self.runtime.get_minor_version() or
equivalent, and initialize status as a dict following the pattern used in
check_rhcos_changes), fix the payload_tags access at line ~1351 so you treat
payload_tags as a sequence of objects (search the sequence for the entry whose
.name matches primary_container and read its rhel_version) instead of doing a
dict .get(...), and update the caller at line ~1359 to await the now-async
check_outdated_rpms call (status = await self.check_outdated_rpms(...)).
doozer/doozerlib/rhcos.py
Outdated
| rhel_version = self.runtime.group_config.rhcos.payload_tags.get(self.primary_container).get("rhel_version") | ||
| url = f"{RHCOS_RELEASES_STREAM_URL}/rhel-{rhel_version}/builds/{build_id}/{self.brew_arch}/commitmeta.json" |
There was a problem hiding this comment.
Fix AttributeError: use self._primary_container instead of self.primary_container.
Line 204 references self.primary_container, but RHCOSBuildFinder stores the parameter as self._primary_container (Line 55). This will cause an AttributeError at runtime.
Apply this diff to fix the attribute name:
if self.layered:
- rhel_version = self.runtime.group_config.rhcos.payload_tags.get(self.primary_container).get("rhel_version")
+ primary_container_name = self.get_primary_container_conf().name if not self._primary_container else self._primary_container
+ rhel_version = self.runtime.group_config.rhcos.payload_tags.get(primary_container_name).get("rhel_version")
url = f"{RHCOS_RELEASES_STREAM_URL}/rhel-{rhel_version}/builds/{build_id}/{self.brew_arch}/commitmeta.json"Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In doozer/doozerlib/rhcos.py around lines 204 to 205, the code references
self.primary_container which does not exist (the class stores it as
self._primary_container); update the reference to use self._primary_container so
the attribute lookup succeeds at runtime and avoid the AttributeError.
| if self.layered: | ||
| # set build_id to the rhel base image build id of the rhel-coreos image | ||
| self.build_id = get_build_id_from_rhcos_pullspec(pullspec_for_tag["rhel-coreos"], layered_id=False) | ||
| self.build_id = ( | ||
| build_id | ||
| if build_id | ||
| else get_build_id_from_rhcos_pullspec(pullspec_for_tag[primary_container], layered_id=False) | ||
| ) | ||
|
|
||
| finder = RHCOSBuildFinder(runtime, self.stream_version, self.brew_arch) | ||
| finder = RHCOSBuildFinder(runtime, self.stream_version, self.brew_arch, primary_container=primary_container) | ||
| self._build_meta = finder.rhcos_build_meta( | ||
| self.build_id, pullspec=pullspec_for_tag.get("rhel-coreos-extensions", None), meta_type='meta' | ||
| self.build_id, pullspec=pullspec_for_tag.get(primary_container + "-extensions", None), meta_type='meta' | ||
| ) | ||
| self._os_commitmeta = finder.rhcos_build_meta( | ||
| self.build_id, pullspec=pullspec_for_tag.get("rhel-coreos", None), meta_type='commitmeta' | ||
| self.build_id, pullspec=pullspec_for_tag.get(primary_container, None), meta_type='commitmeta' | ||
| ) | ||
| self.rhel_build_meta = finder.rhel_build_meta(self.build_id) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that callers pass the correct primary_container parameter.
The layered build logic now correctly uses the primary_container parameter throughout the build metadata retrieval flow. However, the default value "rhel-coreos" (Line 244) might not align with the actual primary container configured in group.yml if callers don't explicitly pass this parameter.
Run the following script to verify all instantiation sites are passing the parameter correctly:
🏁 Script executed:
#!/bin/bash
# Description: Find all RHCOSBuildInspector instantiations and check if primary_container is passed.
# Search for RHCOSBuildInspector instantiations
rg -n -A 5 -B 2 'RHCOSBuildInspector\(' --type pyLength of output: 6780
Update multiple callsites to pass primary_container parameter or verify default alignment with group config.
The new primary_container parameter is used throughout the layered build logic (pullspec construction, metadata retrieval), but verification shows that most instantiation sites are not passing it explicitly, relying instead on the default "rhel-coreos" value. Only scan_sources_konflux.py line 1396 correctly passes the parameter.
The following callsites need to be updated to explicitly pass primary_container:
doozer/doozerlib/assembly_inspector.py:654– passes onlybuild_id, noprimary_containerdoozer/doozerlib/cli/scan_sources.py:660– passes onlybuild_id, noprimary_containerdoozer/doozerlib/cli/get_nightlies.py:421– noprimary_containerpasseddoozer/doozerlib/cli/config.py:453– noprimary_containerpassed
Either explicitly pass the correct primary_container value at each callsite, or verify that the default "rhel-coreos" is correct for all use cases.
🤖 Prompt for AI Agents
In doozer/doozerlib/rhcos.py around lines 254 to 269, the layered build logic
now depends on a primary_container parameter but most callsites still rely on
the implicit default ("rhel-coreos"); update each instantiation to either pass
the correct primary_container or confirm the default is appropriate.
Specifically, modify the following callsites to pass the primary_container used
by that context (or the literal "rhel-coreos" if that is intended):
doozer/doozerlib/assembly_inspector.py line ~654,
doozer/doozerlib/cli/scan_sources.py line ~660,
doozer/doozerlib/cli/get_nightlies.py line ~421, and
doozer/doozerlib/cli/config.py line ~453; ensure the argument is forwarded where
the function creates the RHCOSBuildFinder or calls functions that rely on
primary_container, and add/adjust tests or comments to verify the default is
correct if you choose not to change a callsite.
doozer/doozerlib/rhcos.py
Outdated
| @return Model with entries for name and build_metadata_key | ||
| """ | ||
| return rhcos.get_primary_container_conf(self.runtime) | ||
| return self.primary_container if self.primary_container else rhcos.get_primary_container_conf(self.runtime) |
There was a problem hiding this comment.
Fix type mismatch: return Model object, not string.
Line 415 returns self.primary_container (a string) when truthy, but the method signature and docstring indicate it should return a Model object with name and build_metadata_key fields. This type mismatch will cause an AttributeError at runtime when callers try to access attributes like rhcos_index_tag (see Line 218).
Apply this diff to fix the return type:
def get_primary_container_conf(self):
"""
look up the group.yml-configured primary RHCOS container.
@return Model with entries for name and build_metadata_key
"""
- return self.primary_container if self.primary_container else rhcos.get_primary_container_conf(self.runtime)
+ if not hasattr(self, '_primary_container_conf'):
+ # Look up the config for the specified primary_container name
+ for container_conf in rhcos.get_container_configs(self.runtime):
+ if container_conf.name == self.primary_container:
+ self._primary_container_conf = container_conf
+ break
+ else:
+ # Fallback to default if name not found
+ self._primary_container_conf = rhcos.get_primary_container_conf(self.runtime)
+ return self._primary_container_confCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In doozer/doozerlib/rhcos.py around line 415, the method currently returns
self.primary_container (a string) when truthy but must return a Model object
with name and build_metadata_key fields; change the branch so that if
self.primary_container is truthy you construct and return a Model instance
(importing/using the existing Model class) with name=self.primary_container and
an appropriate build_metadata_key (None or the correct value if available on the
object), otherwise fall back to rhcos.get_primary_container_conf(self.runtime);
ensure the returned type is always Model to avoid AttributeError when callers
access model attributes.
|
@Ximinhan: The following test failed, say
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. |
1 participant
No description provided.