-
Notifications
You must be signed in to change notification settings - Fork 32
fix: making advisories packages alive #1993
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThe PR updates the garbage-collection SQL to expand the alive_qualified_purl CTE by unioning an additional branch that pulls in qualified purls tied to entries in the purl_status table, ensuring advisory-related packages are marked as alive. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- Consider simplifying the CTE by removing the extra parentheses around each SELECT and writing the two branches directly as
SELECT … UNION SELECT …for better readability. - Since UNION already deduplicates rows, double-check that dropping DISTINCT on the first query preserves the intended result or switch to UNION ALL if you don’t need deduplication for performance.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider simplifying the CTE by removing the extra parentheses around each SELECT and writing the two branches directly as `SELECT … UNION SELECT …` for better readability.
- Since UNION already deduplicates rows, double-check that dropping DISTINCT on the first query preserves the intended result or switch to UNION ALL if you don’t need deduplication for performance.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1993 +/- ##
=======================================
Coverage 68.00% 68.00%
=======================================
Files 355 355
Lines 19804 19804
Branches 19804 19804
=======================================
Hits 13467 13467
+ Misses 5557 5556 -1
- Partials 780 781 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jcrossley3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a unit test that affirms the change actually fixes the problem.
|
Probably better to add this file change in your PR that contains the test? ────────────
Nextest run ID 595cbdcc-f713-4727-9033-48756ce95de4 with nextest profile: default
Starting 1 test across 35 binaries (478 tests skipped)
PASS [ 2.735s] trustify-module-fundamental purl::service::test::gc_purls_from_single_csaf
────────────
Summary [ 2.774s] 1 test run: 1 passed, 478 skipped
➜ trustify git:(1977) ✗ git diff
diff --git a/modules/fundamental/src/purl/service/gc_purls.sql b/modules/fundamental/src/purl/service/gc_purls.sql
index 4a8703a2..4c07ea97 100644
--- a/modules/fundamental/src/purl/service/gc_purls.sql
+++ b/modules/fundamental/src/purl/service/gc_purls.sql
@@ -1,9 +1,21 @@
WITH
alive_qualified_purl AS (
- SELECT DISTINCT t2.id, t2.versioned_purl_id
+ (
+ SELECT t2.id, t2.versioned_purl_id
FROM sbom_package_purl_ref AS t1
INNER JOIN qualified_purl AS t2
ON t2.id = t1.qualified_purl_id
+ )
+ UNION
+ (
+ SELECT t1.id, t1.versioned_purl_id
+ FROM qualified_purl AS t1
+ INNER JOIN versioned_purl AS t2 ON t2.id = t1.versioned_purl_id
+ INNER JOIN (
+ SELECT DISTINCT base_purl_id
+ FROM purl_status
+ ) AS t3 ON t2.base_purl_id = t3.base_purl_id
+ )
),
alive_versioned_purl AS (
SELECT DISTINCT t2.id, t2.base_purl_id |
|
PR, which contains the test, updated #1980 👍 |
Summary by Sourcery
Enhancements: