Modular RPM support fix - #89
Conversation
rockythorn
left a comment
There was a problem hiding this comment.
Review: Request changes (one blocking issue)
Tests pass (test_rh_matcher_activities, test_rhcsaf), no merge conflicts with main. Code is well-structured.
What works
CSAF parsing: _strip_csaf_product_prefix(), _module_fields_from_purl(), and _parse_csaf_package_product_id() correctly extract module metadata from CSAF product IDs and purls. The AppStream prefix stripping and ::module:stream suffix handling are both tested.
Module resolution fallback: _resolve_modular_package_fields has a clean multi-step fallback: modules.yaml -> RH CSAF advisory data -> enrichment from yaml for missing version/context. The .rocky suffix normalization in _normalize_module_nevra_key handles the Rocky rebuild NEVRA mismatch.
Worker integration: create_or_update_red_hat_advisory_packages correctly handles both new packages (with module fields in bulk_create) and existing packages (selective update only when module fields are missing).
Datetime formats: Adding microsecond format (%f) to parse_datetime prevents crashes on fractional-second timestamps from CSAF.
Blocking issue
Missing database migration. The PR adds 4 columns to RedHatAdvisoryPackage:
module_context = fields.TextField(null=True)
module_name = fields.TextField(null=True)
module_stream = fields.TextField(null=True)
module_version = fields.TextField(null=True)But these columns don't exist in the actual database:
\d red_hat_advisory_packages
id | bigint
red_hat_advisory_id | bigint
nevra | text
The worker will crash on any INSERT/UPDATE that references these columns. Needs a migration:
ALTER TABLE red_hat_advisory_packages
ADD COLUMN IF NOT EXISTS module_context text,
ADD COLUMN IF NOT EXISTS module_name text,
ADD COLUMN IF NOT EXISTS module_stream text,
ADD COLUMN IF NOT EXISTS module_version text;Either include this in the PR or document that it must be applied before deployment.
We were discarding ::nodejs:16 from RH CSAF at ingest, then trying to rediscover it via exact NEVRA match in Rocky’s modules.yaml. That breaks when Rocky’s build ID (+1760+903d54b9) doesn’t match Red Hat’s (+21536+8fdee1fb). Fix is keep the stream from CSAF and match without requiring identical build IDs. a specific example, the redhat data looks like this: nodejs-1:16.20.2-4.module+el8.9.0+21536+8fdee1fb.x86_64::nodejs:16 and apollo was discarding "::nodejs:16", so it just contained the bare NERVA. Then later when apollo tries to reconstruct the module data, its looking for the exact redhat NERVA, which is never going to be true, since we build the module with a different build ID. So the lookup misses, and the RLSA packages end up with module_name / stream null even though Red Hat told us the stream in the CSAF. Fix: keep the module identity from CSAF at ingest, and when yaml doesn’t match, fall back using a cleaned NEVRA (ignoring the build-id suffix) so Rocky’s rebuild still maps to nodejs:16. Closes issue resf#72 Signed-off-by: Scott R. Shinn <scott@atomicorp.com>
ORM already declares module_*; without the columns the worker crashes on INSERT/UPDATE.
|
Addressed review feedback and rebased onto `main`:
|
33db4d7 to
4e442b4
Compare
rockythorn
left a comment
There was a problem hiding this comment.
Re-review: Approve
Migration added: apollo/migrations/20260903172301_add_rh_advisory_package_module_fields.sql
- Correct
ALTER TABLEwithIF NOT EXISTSguards - Proper
migrate:up/migrate:downsections schema.sqlupdated to match
bazel test //apollo/tests:test_rh_matcher_activities //apollo/tests:test_rhcsaf — both pass.
We were discarding ::nodejs:16 from RH CSAF at ingest, then trying to rediscover it via exact NEVRA match in Rocky’s modules.yaml. That breaks when Rocky’s build ID (+1760+903d54b9) doesn’t match Red Hat’s (+21536+8fdee1fb). Fix is keep the stream from CSAF and match without requiring identical build IDs.
a specific example, the redhat data looks like this:
nodejs-1:16.20.2-4.module+el8.9.0+21536+8fdee1fb.x86_64::nodejs:16
and apollo was discarding "::nodejs:16", so it just contained the bare NERVA. Then later when apollo tries to reconstruct the module data, its looking for the exact redhat NERVA, which is never going to be true, since we build the module with a different build ID. So the lookup misses, and the RLSA packages end up with module_name / stream null even though Red Hat told us the stream in the CSAF.
Fix: keep the module identity from CSAF at ingest, and when yaml doesn’t match, fall back using a cleaned NEVRA (ignoring the build-id suffix) so Rocky’s rebuild still maps to nodejs:16.
Closes issue #72