ROX-32714: Enable feature flag by default (Still updating failing tests. Do not review)#18539
Open
charmik-redhat wants to merge 15 commits intomasterfrom
Open
ROX-32714: Enable feature flag by default (Still updating failing tests. Do not review)#18539charmik-redhat wants to merge 15 commits intomasterfrom
charmik-redhat wants to merge 15 commits intomasterfrom
Conversation
Contributor
|
Images are ready for the commit at 6a2db38. To use with deploy scripts, first |
charmik-redhat
force-pushed
the
ROX-32714/enable-flatten-image-data-flag
branch
from
4de3b21 to
f90b80a
Compare
charmik-redhat
force-pushed
the
ROX-32714/enable-flatten-image-data-flag
branch
from
bedba7d to
531cfdf
Compare
charmik-redhat
force-pushed
the
ROX-32714/enable-flatten-image-data-flag
branch
from
531cfdf to
48fa70f
Compare
Contributor
Author
|
/retest |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
charmik-redhat
force-pushed
the
ROX-32714/enable-flatten-image-data-flag
branch
from
bed2196 to
1dfb3d6
Compare
Contributor
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThe pull request adds conditional support for a flattened v2 image data model controlled by the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
central/views/imagecveflat/view_test.go (1)
145-197: Drive this suite with an explicit flag mode instead of ambient global state.After the default flip, normal CI will only exercise one of these setup branches, so the other path becomes dead test scaffolding. The helpers also keep consulting the global flag after setup. If both modes still matter, parameterize the suite and force the flag on/off per run; otherwise cache the chosen mode on
ImageCVEFlatViewTestSuiteand drop the unused branch.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 211-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@central/views/imagecveflat/view_test.go` around lines 145 - 197, The test setup branches are driven by the global features.FlattenImageData flag which makes one branch dead in CI; add an explicit mode on the test suite (e.g., a bool field on ImageCVEFlatViewTestSuite like flattenMode) and use that instead of calling features.FlattenImageData.Enabled() in the setup block; set flattenMode when registering/running the suite to force the desired branch for each run, then update any helpers and later checks that currently call features.FlattenImageData.Enabled() to read s.flattenMode (affecting the setup code paths using imageV2DS.GetTestPostgresDataStore, imageDS.GetTestPostgresDataStore, setCVSSMetrics, and construction of s.testImages/deployments).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@central/views/imagecveflat/view_test.go`:
- Around line 145-197: The test setup branches are driven by the global
features.FlattenImageData flag which makes one branch dead in CI; add an
explicit mode on the test suite (e.g., a bool field on ImageCVEFlatViewTestSuite
like flattenMode) and use that instead of calling
features.FlattenImageData.Enabled() in the setup block; set flattenMode when
registering/running the suite to force the desired branch for each run, then
update any helpers and later checks that currently call
features.FlattenImageData.Enabled() to read s.flattenMode (affecting the setup
code paths using imageV2DS.GetTestPostgresDataStore,
imageDS.GetTestPostgresDataStore, setCVSSMetrics, and construction of
s.testImages/deployments).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: c6e5ce07-cfbe-4ae4-9830-62b2dc9c2f48
📒 Files selected for processing (13)
central/deployment/datastore/internal/store/postgres/full_store.gocentral/deployment/datastore/test/datastore_sac_test.gocentral/graphql/resolvers/images_test.gocentral/splunk/vm.gocentral/views/deployments/view_test.gocentral/views/imagecve/view_test.gocentral/views/imagecveflat/view_test.gopkg/features/list.gopkg/fixtures/image/reader.gopkg/jsonutil/test/testdata/compat.alert.jsonpkg/jsonutil/test/testdata/pretty.alert.jsonroxctl/central/export/deployments/serialized_test_deployment.jsontests/e2e/lib.sh
💤 Files with no reviewable changes (4)
- central/splunk/vm.go
- central/graphql/resolvers/images_test.go
- central/deployment/datastore/test/datastore_sac_test.go
- central/deployment/datastore/internal/store/postgres/full_store.go
charmik-redhat
force-pushed
the
ROX-32714/enable-flatten-image-data-flag
branch
from
34cfdcc to
69e578f
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@central/views/deployments/view_test.go`:
- Around line 133-146: The test currently ignores the persisted ImageV2 returned
by imageV2Store.GetImage and continues using the pre-conversion *storage.Image
fixture (via imageUtils.ConvertToV2), which lets conversion/persistence bugs
slip through; change the loop so that after calling
imageV2Store.GetImage(imageV2ID) you use the retrieved V2 record as the source
of truth (or derive expectations from the component/CVE tables) — i.e.,
standardize and assign images[idx] from the fetched V2 payload instead of the
original pre-conversion fixture, keeping calls to imageV2Store.GetImage,
imageUtils.ConvertToV2, and standardizeImages as reference points when updating
the logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 08dd3757-4037-4a76-bde3-ea3032cd378c
📒 Files selected for processing (15)
central/deployment/datastore/internal/store/postgres/full_store.gocentral/deployment/datastore/test/datastore_sac_test.gocentral/graphql/resolvers/images_test.gocentral/splunk/vm.gocentral/views/deployments/view_test.gocentral/views/imagecve/view_test.gocentral/views/imagecveflat/view_test.gopkg/features/list.gopkg/fixtures/image/reader.gopkg/jsonutil/test/testdata/compat.alert.jsonpkg/jsonutil/test/testdata/pretty.alert.jsonroxctl/central/export/deployments/serialized_test_deployment.jsontests/e2e/lib.shui/apps/platform/cypress/fixtures/vulnerabilities/workloadCves/multipleCvesForImageV2.jsonui/apps/platform/cypress/integration/vulnerabilities/workloadCves/WorkloadCves.helpers.js
💤 Files with no reviewable changes (4)
- central/splunk/vm.go
- central/graphql/resolvers/images_test.go
- central/deployment/datastore/internal/store/postgres/full_store.go
- central/deployment/datastore/test/datastore_sac_test.go
✅ Files skipped from review due to trivial changes (4)
- pkg/fixtures/image/reader.go
- roxctl/central/export/deployments/serialized_test_deployment.json
- tests/e2e/lib.sh
- ui/apps/platform/cypress/fixtures/vulnerabilities/workloadCves/multipleCvesForImageV2.json
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/jsonutil/test/testdata/pretty.alert.json
- pkg/jsonutil/test/testdata/compat.alert.json
- pkg/features/list.go
- central/views/imagecve/view_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #18539 +/- ##
==========================================
- Coverage 49.58% 49.26% -0.33%
==========================================
Files 2766 2768 +2
Lines 208540 208612 +72
==========================================
- Hits 103408 102776 -632
- Misses 97460 98297 +837
+ Partials 7672 7539 -133
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pedrottimark
reviewed
Apr 1, 2026
| ci_export ROX_NETWORK_GRAPH_AGGREGATE_EXT_IPS "${ROX_NETWORK_GRAPH_AGGREGATE_EXT_IPS:-true}" | ||
| ci_export ROX_NETWORK_GRAPH_EXTERNAL_IPS "${ROX_NETWORK_GRAPH_EXTERNAL_IPS:-false}" | ||
| ci_export ROX_FLATTEN_IMAGE_DATA "${ROX_FLATTEN_IMAGE_DATA:-false}" | ||
| ci_export ROX_FLATTEN_IMAGE_DATA "${ROX_FLATTEN_IMAGE_DATA:-true}" |
Contributor
There was a problem hiding this comment.
For our future selves, the procedure is 2 occurrrences of true in lib.sh file when we add a feature flag, so integration tests work when enabled in pkg/features/list.go file.
Unexpected test failures almost because a last minute blocker to enable base images feature.
charmik-redhat
force-pushed
the
ROX-32714/enable-flatten-image-data-flag
branch
from
94454b2 to
7496043
Compare
Contributor
🚀 Build Images ReadyImages are ready for commit 4b48b2b. To use with deploy scripts: export MAIN_IMAGE_TAG=4.11.x-601-g4b48b2bd12 |
Contributor
Author
|
/retest |
Contributor
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 (2)
central/views/imagecveflat/view_test.go (1)
565-577:⚠️ Potential issue | 🟠 MajorBuild the parent CVE scope from the same image as the outer scope.
Line 576 uses
wordpressLatest.GetId()while the enclosing image scope iswordpressDebian. V2 component/CVE IDs include the image ID, so once flattening is enabled those scopes stop referring to the same record and this case can empty out unexpectedly.Possible fix
- }, wordpressLatest.GetId(), 0), 0)}, + }, wordpressDebian.GetId(), 0), 0)},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@central/views/imagecveflat/view_test.go` around lines 565 - 577, The parent CVE scope is built using wordpressLatest.GetId() while the outer scope uses wordpressDebian, causing mismatched V2 component/CVE IDs; update the parent scope construction so getTestComponentID(..., wordpressLatest.GetId(), 0) becomes getTestComponentID(..., wordpressDebian.GetId(), 0) (i.e., ensure the parent scoped.Scope uses wordpressDebian.GetId()) so getTestCVEID/getTestComponentID, scoped.Context and scoped.Scope refer to the same image.central/views/imagecve/view_test.go (1)
1277-1294:⚠️ Potential issue | 🟠 MajorQuery
TestImageCVEUnknownSeveritywith the mode-specific image key.When
FlattenImageDatais enabled, Lines 1279-1289 insert V2 images, but Lines 1292-1294 still search withsearch.ImageSHA = "sha1". That no longer addresses the inserted record, so the unknown-severity assertions can go false-negative as soon as the flag is on.Possible fix
// Upsert test images using the appropriate datastore. images := testImages() + queryID := "sha1" if features.FlattenImageData.Enabled() { imageV2Store := imageV2DS.GetTestPostgresDataStore(t, testDB.DB) for _, image := range images { - assert.NoError(t, imageV2Store.UpsertImage(ctx, imageUtils.ConvertToV2(image))) + v2 := imageUtils.ConvertToV2(image) + if image.GetId() == "sha1" { + queryID = v2.GetId() + } + assert.NoError(t, imageV2Store.UpsertImage(ctx, v2)) } } else { imageStore := imageDS.GetTestPostgresDataStore(t, testDB.DB) for _, image := range images { assert.NoError(t, imageStore.UpsertImage(ctx, image)) @@ cveView := NewCVEView(testDB.DB) query := search.NewQueryBuilder(). - AddExactMatches(search.ImageSHA, "sha1"). + AddExactMatches(imageSearchField(), queryID). ProtoQuery()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@central/views/imagecve/view_test.go` around lines 1277 - 1294, The test inserts images via imageStore.UpsertImage or imageV2Store.UpsertImage depending on features.FlattenImageData, but the query always uses AddExactMatches(search.ImageSHA, "sha1"); change the test to pick the mode-specific search key based on features.FlattenImageData.Enabled() (e.g., use search.ImageSHA when false and the V2 image key when true) and call AddExactMatches with that key so the query targets the record you inserted; update the block around NewCVEView and search.NewQueryBuilder().AddExactMatches(...) to select the correct search key at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@central/cve/image/v2/datastore/store/postgres/store.go`:
- Line 105: The code is passing pgutils.NilOrString(obj.GetImageId()) into the
DB while the generated GORM struct for ImageID in image_cves_v2.go is a
non-nullable string, causing NOT NULL constraint violations; regenerate the GORM
schema so the proto's allow-null for image_id is respected (ImageID should
become *string), or update the generator/schema so ImageID is a pointer type,
then re-run code generation to sync image_cves_v2.go with cve.proto and ensure
upserts using pgutils.NilOrString(...)/pgutils.NilOrString(obj.GetImageId())
work without violating constraints.
In `@qa-tests-backend/src/test/groovy/CSVTest.groovy`:
- Around line 238-240: The CSV test filters still use the hardcoded digest
(TEST_IMAGE_SHA) while the test's id can be TEST_IMAGE_V2_ID and images are
flattened; update the CSV filter to use the same flattened image identity used
by the GraphQL call (e.g., build the query string from getTestImageId() or
TEST_IMAGE_V2_ID instead of TEST_IMAGE_SHA) so the CSV export and GraphQL lookup
reference the identical image key (adjust the line constructing "Image
Sha:${TEST_IMAGE_SHA}+Fixable:true" to use the flattened id variable).
---
Outside diff comments:
In `@central/views/imagecve/view_test.go`:
- Around line 1277-1294: The test inserts images via imageStore.UpsertImage or
imageV2Store.UpsertImage depending on features.FlattenImageData, but the query
always uses AddExactMatches(search.ImageSHA, "sha1"); change the test to pick
the mode-specific search key based on features.FlattenImageData.Enabled() (e.g.,
use search.ImageSHA when false and the V2 image key when true) and call
AddExactMatches with that key so the query targets the record you inserted;
update the block around NewCVEView and
search.NewQueryBuilder().AddExactMatches(...) to select the correct search key
at runtime.
In `@central/views/imagecveflat/view_test.go`:
- Around line 565-577: The parent CVE scope is built using
wordpressLatest.GetId() while the outer scope uses wordpressDebian, causing
mismatched V2 component/CVE IDs; update the parent scope construction so
getTestComponentID(..., wordpressLatest.GetId(), 0) becomes
getTestComponentID(..., wordpressDebian.GetId(), 0) (i.e., ensure the parent
scoped.Scope uses wordpressDebian.GetId()) so getTestCVEID/getTestComponentID,
scoped.Context and scoped.Scope refer to the same image.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: c99715a1-814e-4182-bcdb-fa7438b494f6
⛔ Files ignored due to path filters (2)
generated/storage/cve.pb.gois excluded by!**/*.pb.go,!**/generated/**generated/storage/image_component.pb.gois excluded by!**/*.pb.go,!**/generated/**
📒 Files selected for processing (27)
central/cve/image/v2/datastore/store/postgres/store.gocentral/image/datastore/singleton.gocentral/image/service/service_impl.gocentral/image/service/service_impl_test.gocentral/imagecomponent/v2/datastore/store/postgres/store.gocentral/views/deployments/view_test.gocentral/views/imagecve/view_test.gocentral/views/imagecveflat/view_test.gopkg/features/list.gopkg/fixtures/image/reader.gopkg/jsonutil/test/testdata/compat.alert.jsonpkg/jsonutil/test/testdata/pretty.alert.jsonproto/storage/cve.protoproto/storage/image_component.protoqa-tests-backend/src/main/groovy/util/Helpers.groovyqa-tests-backend/src/test/groovy/AdmissionControllerTest.groovyqa-tests-backend/src/test/groovy/BaseSpecification.groovyqa-tests-backend/src/test/groovy/CSVTest.groovyqa-tests-backend/src/test/groovy/GlobalSearch.groovyqa-tests-backend/src/test/groovy/ImageManagementTest.groovyqa-tests-backend/src/test/groovy/SACTest.groovyqa-tests-backend/src/test/groovy/VulnMgmtTest.groovyroxctl/central/export/deployments/serialized_test_deployment.jsontests/e2e/lib.shui/apps/platform/cypress/fixtures/vulnerabilities/workloadCves/multipleCvesForImageV2.jsonui/apps/platform/cypress/integration/vulnerabilities/workloadCves/WorkloadCves.helpers.jsui/apps/platform/cypress/integration/vulnerabilities/workloadCves/imageSingle.test.ts
💤 Files with no reviewable changes (1)
- central/image/datastore/singleton.go
✅ Files skipped from review due to trivial changes (4)
- pkg/jsonutil/test/testdata/pretty.alert.json
- roxctl/central/export/deployments/serialized_test_deployment.json
- pkg/jsonutil/test/testdata/compat.alert.json
- ui/apps/platform/cypress/fixtures/vulnerabilities/workloadCves/multipleCvesForImageV2.json
🚧 Files skipped from review as they are similar to previous changes (5)
- ui/apps/platform/cypress/integration/vulnerabilities/workloadCves/WorkloadCves.helpers.js
- pkg/features/list.go
- tests/e2e/lib.sh
- pkg/fixtures/image/reader.go
- central/views/deployments/view_test.go
charmik-redhat
force-pushed
the
ROX-32714/enable-flatten-image-data-flag
branch
from
126fc01 to
09f9a74
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
charmik-redhat
force-pushed
the
ROX-32714/enable-flatten-image-data-flag
branch
from
09f9a74 to
8770500
Compare
|
@charmik-redhat: The following tests 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Updating failing tests. Do not review yet.
User-facing documentation
Testing and quality
Automated testing
How I validated my change
change me!