Skip to content

Commit 76de4c3

Browse files
committed
fix: address review findings in LP pipeline PR
- Fix pullspec parsing to preserve registry ports (rsplit vs split) - Raise RuntimeError on unknown operator identity in FBC validation instead of silently skipping the consistency check - Fix test_extract_nvrs_skips_validation_for_single_pullspec to actually exercise the single-pullspec code path - Document single-level inheritance limitation in docstrings Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
1 parent 2d9c931 commit 76de4c3

5 files changed

Lines changed: 28 additions & 15 deletions

File tree

doozer/doozerlib/cli/fbc.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -679,7 +679,7 @@ async def _rebase_and_build(
679679
results = pipelinerun_dict.get('status', {}).get('results', [])
680680
image_url = next((r['value'] for r in results if r['name'] == 'IMAGE_URL'), None)
681681
image_digest = next((r['value'] for r in results if r['name'] == 'IMAGE_DIGEST'), None)
682-
pullspec = f"{image_url.split(':')[0]}@{image_digest}" if image_url and image_digest else ""
682+
pullspec = f"{image_url.rsplit(':', 1)[0]}@{image_digest}" if image_url and image_digest else ""
683683
if not pullspec:
684684
LOGGER.warning("Could not extract pullspec from pipelinerun results for %s", nvr)
685685
return nvr, pullspec

pyartcd/pyartcd/fbc_util.py

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -116,16 +116,14 @@ async def validate_fbc_related_images(fbc_pullspecs: List[str], product: str) ->
116116
operator,
117117
)
118118
else:
119-
operator = f"UNKNOWN-{hash(fbc_pullspec) & 0xFFFFFFFF:08x}"
120-
logger.error(
121-
"Could not determine operator for %s (nvr=%s), using: %s",
122-
fbc_pullspec,
123-
nvr,
124-
operator,
119+
raise RuntimeError(
120+
f"Could not determine operator for FBC {fbc_pullspec} (nvr={nvr}). "
121+
f"Cannot validate related-image consistency without operator identity."
125122
)
123+
except RuntimeError:
124+
raise
126125
except Exception as e:
127-
logger.error("Failed to extract operator from %s: %s", fbc_pullspec, e)
128-
operator = f"UNKNOWN-{hash(fbc_pullspec) & 0xFFFFFFFF:08x}"
126+
raise RuntimeError(f"Failed to extract operator from {fbc_pullspec}: {e}") from e
129127

130128
fbc_to_operator[fbc_pullspec] = operator
131129

pyartcd/pyartcd/pipelines/gen_assembly_lp.py

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,12 @@ def _apply_extra_image_nvrs(self, operand_map: Dict[str, str]) -> Dict[str, str]
217217
return operand_map
218218

219219
async def _resolve_parent_operands(self) -> Dict[str, str]:
220-
"""Resolve the full operand set from the parent (basis) assembly."""
220+
"""Resolve the full operand set from the parent (basis) assembly.
221+
222+
NOTE: Only resolves one level of inheritance. Multi-level chains
223+
(A->B->C) are not supported; the basis assembly must list all
224+
operands explicitly.
225+
"""
221226
self._logger.info("Resolving operand set from basis assembly '%s'...", self.basis_assembly)
222227

223228
build_data_path = self._working_dir / "ocp-build-data-read"

pyartcd/pyartcd/pipelines/prepare_release_lp.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -222,6 +222,11 @@ def _extract_operand_nvrs(self, assembly_config: Dict) -> List[str]:
222222
"""Extract pinned operand NVRs from the assembly definition.
223223
224224
Walks the members.images list and collects NVRs from is.nvr or is.nvr! entries.
225+
226+
NOTE: For inherited (delta) assemblies, only NVRs explicitly listed
227+
in this assembly's members.images are returned. Parent chain
228+
resolution is not performed -- doozer/elliott commands handle
229+
inheritance internally via --assembly.
225230
"""
226231
members_images = assembly_config.get('assembly', {}).get('members', {}).get('images', [])
227232
nvrs = []

pyartcd/tests/pipelines/test_gen_assembly_lp.py

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -291,18 +291,23 @@ def test_extract_nvrs_calls_validation_for_multiple_pullspecs(self, mock_validat
291291
self.assertIn("cluster-logging-operator", result)
292292

293293
@patch('pyartcd.pipelines.gen_assembly_lp.validate_fbc_related_images', new_callable=AsyncMock)
294-
def test_extract_nvrs_skips_validation_for_single_pullspec(self, mock_validate):
294+
@patch('elliottlib.util.extract_nvrs_from_fbc', new_callable=AsyncMock)
295+
def test_extract_nvrs_skips_validation_for_single_pullspec(self, mock_extract_nvrs, mock_validate):
295296
"""When only one FBC pullspec is provided, skip validation and extract directly."""
297+
mock_extract_nvrs.return_value = [
298+
"cluster-logging-operator-6.4.1-1",
299+
"log-file-metric-exporter-6.4.1-1",
300+
]
296301
pipeline = self._make_pipeline(
297302
fbc_pullspecs=["quay.io/test/fbc@sha256:single"],
298303
)
299304

300-
with patch('pyartcd.pipelines.gen_assembly_lp.GenAssemblyLPPipeline._extract_nvrs_from_fbc') as mock_extract:
301-
mock_extract.return_value = {"some-container": "some-container-6.4.1-1"}
302-
# Just verify the method exists and is callable
303-
self.assertTrue(callable(pipeline._extract_nvrs_from_fbc))
305+
result = asyncio.run(pipeline._extract_nvrs_from_fbc())
304306

305307
mock_validate.assert_not_called()
308+
mock_extract_nvrs.assert_called_once_with("quay.io/test/fbc@sha256:single", "openshift-logging")
309+
self.assertIn("cluster-logging-operator", result)
310+
self.assertIn("log-file-metric-exporter", result)
306311

307312
@patch('pyartcd.pipelines.gen_assembly_lp.extract_fbc_labels', new_callable=AsyncMock)
308313
def test_build_fbc_pullspecs_map(self, mock_labels):

0 commit comments

Comments
 (0)