NETOBSERV-2701 handle plugin images from CSV#2598
Open
jpinsonneau wants to merge 1 commit intonetobserv:mainfrom
Open
NETOBSERV-2701 handle plugin images from CSV#2598jpinsonneau wants to merge 1 commit intonetobserv:mainfrom
jpinsonneau wants to merge 1 commit intonetobserv:mainfrom
Conversation
|
Skipping CI for Draft Pull Request. |
|
[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 |
📝 WalkthroughWalkthroughThe PR refactors console plugin image management from a fixed single/compat image approach to a version-aware mapping system that selects plugin variants based on OpenShift version thresholds, replacing hardcoded image flags with a semicolon-separated configuration format. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2598 +/- ##
==========================================
- Coverage 72.39% 72.23% -0.17%
==========================================
Files 105 105
Lines 10851 10883 +32
==========================================
+ Hits 7856 7861 +5
- Misses 2518 2538 +20
- Partials 477 484 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
jpinsonneau
added
the
ok-to-test
|
New images: quay.io/netobserv/network-observability-operator:919fc3c
quay.io/netobserv/network-observability-operator-bundle:v0.0.0-sha-919fc3c
quay.io/netobserv/network-observability-operator-catalog:v0.0.0-sha-919fc3cThey will expire in two weeks. To deploy this build: # Direct deployment, from operator repo
IMAGE=quay.io/netobserv/network-observability-operator:919fc3c make deploy
# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-sha-919fc3cOr as a Catalog Source: apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
name: netobserv-dev
namespace: openshift-marketplace
spec:
sourceType: grpc
image: quay.io/netobserv/network-observability-operator-catalog:v0.0.0-sha-919fc3c
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/pkg/manager/config.go`:
- Around line 87-96: The validation for cfg.ConsolePluginImageVariants must
ensure each MinVersion is a valid OCP version and that the slice is strictly
ascending so ResolveConsolePluginImage behaves deterministically; in the loop in
config.go (where ConsolePluginImageVariants is checked) parse/validate each
v.MinVersion using the same version parser/semver logic used by
ResolveConsolePluginImage and return an error if parsing fails, and additionally
compare each parsed MinVersion to the previous one to ensure it is strictly
greater, returning a clear fmt.Errorf referencing the offending index (use the
same symbols ConsolePluginImageVariants and ResolveConsolePluginImage to locate
code).
- Around line 41-59: ParseConsolePluginImages mutates
cfg.ConsolePluginImageVariants incrementally, causing partial updates on errors
and retention across calls; fix by building a new local slice (e.g.,
newVariants) inside ParseConsolePluginImages, parse and validate all entries
into that slice without touching cfg, and only if parsing completes successfully
replace cfg.ConsolePluginImageVariants = newVariants so the update is atomic and
leaves the original slice intact on error.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eb51c863-b3fa-448f-8866-77105b3e23e8
📒 Files selected for processing (11)
Makefilebundle/manifests/netobserv-operator.clusterserviceversion.yamlconfig/manager/manager.yamlinternal/controller/consoleplugin/consoleplugin_objects.gointernal/controller/flowcollector_controller.gointernal/controller/reconcilers/common.gointernal/controller/static/static_controller.gointernal/pkg/manager/config.gointernal/pkg/manager/config_test.gointernal/pkg/test/envtest.gomain.go
Comment on lines
+41
to
+59
| func (cfg *Config) ParseConsolePluginImages(raw string) error { | ||
| if raw == "" { | ||
| return nil | ||
| } | ||
| for _, entry := range strings.Split(raw, ";") { | ||
| entry = strings.TrimSpace(entry) | ||
| if entry == "" { | ||
| continue | ||
| } | ||
| eqIdx := strings.Index(entry, "=") | ||
| if eqIdx <= 0 || eqIdx >= len(entry)-1 { | ||
| return fmt.Errorf("invalid console plugin image entry %q: expected format minVersion=image", entry) | ||
| } | ||
| cfg.ConsolePluginImageVariants = append(cfg.ConsolePluginImageVariants, ConsolePluginImageVariant{ | ||
| MinVersion: entry[:eqIdx], | ||
| Image: entry[eqIdx+1:], | ||
| }) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Replace the variant slice atomically.
This parser mutates ConsolePluginImageVariants as it goes. If it is called twice, old entries are retained, and a malformed later token leaves the config partially updated.
Suggested fix
func (cfg *Config) ParseConsolePluginImages(raw string) error {
- if raw == "" {
+ var variants []ConsolePluginImageVariant
+ if strings.TrimSpace(raw) == "" {
+ cfg.ConsolePluginImageVariants = nil
return nil
}
for _, entry := range strings.Split(raw, ";") {
entry = strings.TrimSpace(entry)
if entry == "" {
@@
if eqIdx <= 0 || eqIdx >= len(entry)-1 {
return fmt.Errorf("invalid console plugin image entry %q: expected format minVersion=image", entry)
}
- cfg.ConsolePluginImageVariants = append(cfg.ConsolePluginImageVariants, ConsolePluginImageVariant{
+ variants = append(variants, ConsolePluginImageVariant{
MinVersion: entry[:eqIdx],
Image: entry[eqIdx+1:],
})
}
+ cfg.ConsolePluginImageVariants = variants
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (cfg *Config) ParseConsolePluginImages(raw string) error { | |
| if raw == "" { | |
| return nil | |
| } | |
| for _, entry := range strings.Split(raw, ";") { | |
| entry = strings.TrimSpace(entry) | |
| if entry == "" { | |
| continue | |
| } | |
| eqIdx := strings.Index(entry, "=") | |
| if eqIdx <= 0 || eqIdx >= len(entry)-1 { | |
| return fmt.Errorf("invalid console plugin image entry %q: expected format minVersion=image", entry) | |
| } | |
| cfg.ConsolePluginImageVariants = append(cfg.ConsolePluginImageVariants, ConsolePluginImageVariant{ | |
| MinVersion: entry[:eqIdx], | |
| Image: entry[eqIdx+1:], | |
| }) | |
| } | |
| return nil | |
| func (cfg *Config) ParseConsolePluginImages(raw string) error { | |
| var variants []ConsolePluginImageVariant | |
| if strings.TrimSpace(raw) == "" { | |
| cfg.ConsolePluginImageVariants = nil | |
| return nil | |
| } | |
| for _, entry := range strings.Split(raw, ";") { | |
| entry = strings.TrimSpace(entry) | |
| if entry == "" { | |
| continue | |
| } | |
| eqIdx := strings.Index(entry, "=") | |
| if eqIdx <= 0 || eqIdx >= len(entry)-1 { | |
| return fmt.Errorf("invalid console plugin image entry %q: expected format minVersion=image", entry) | |
| } | |
| variants = append(variants, ConsolePluginImageVariant{ | |
| MinVersion: entry[:eqIdx], | |
| Image: entry[eqIdx+1:], | |
| }) | |
| } | |
| cfg.ConsolePluginImageVariants = variants | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/pkg/manager/config.go` around lines 41 - 59,
ParseConsolePluginImages mutates cfg.ConsolePluginImageVariants incrementally,
causing partial updates on errors and retention across calls; fix by building a
new local slice (e.g., newVariants) inside ParseConsolePluginImages, parse and
validate all entries into that slice without touching cfg, and only if parsing
completes successfully replace cfg.ConsolePluginImageVariants = newVariants so
the update is atomic and leaves the original slice intact on error.
Comment on lines
+87
to
+96
| if len(cfg.ConsolePluginImageVariants) == 0 { | ||
| return errors.New("console plugin images can't be empty") | ||
| } | ||
| for i, v := range cfg.ConsolePluginImageVariants { | ||
| if v.Image == "" { | ||
| return fmt.Errorf("console plugin image variant %d has empty image", i) | ||
| } | ||
| if v.MinVersion == "" { | ||
| return fmt.Errorf("console plugin image variant %d has empty MinVersion", i) | ||
| } |
There was a problem hiding this comment.
Reject misordered or invalid thresholds during validation.
ResolveConsolePluginImage only behaves correctly when every MinVersion is a valid OCP version and the slice is strictly ascending. Right now any non-empty string passes validation, so bad CSV content can silently resolve to the wrong plugin image.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/pkg/manager/config.go` around lines 87 - 96, The validation for
cfg.ConsolePluginImageVariants must ensure each MinVersion is a valid OCP
version and that the slice is strictly ascending so ResolveConsolePluginImage
behaves deterministically; in the loop in config.go (where
ConsolePluginImageVariants is checked) parse/validate each v.MinVersion using
the same version parser/semver logic used by ResolveConsolePluginImage and
return an error if parsing fails, and additionally compare each parsed
MinVersion to the previous one to ensure it is strictly greater, returning a
clear fmt.Errorf referencing the offending index (use the same symbols
ConsolePluginImageVariants and ResolveConsolePluginImage to locate code).
memodi
reviewed
Mar 27, 2026
| - --flowlogs-pipeline-image=$(RELATED_IMAGE_FLOWLOGS_PIPELINE) | ||
| - --console-plugin-image=$(RELATED_IMAGE_CONSOLE_PLUGIN) | ||
| - --console-plugin-compat-image=$(RELATED_IMAGE_CONSOLE_PLUGIN_COMPAT) | ||
| - --console-plugin-images=4.0.0=$(RELATED_IMAGE_CONSOLE_PLUGIN_PF4);4.15.0=$(RELATED_IMAGE_CONSOLE_PLUGIN_PF5);4.22.0=$(RELATED_IMAGE_CONSOLE_PLUGIN) |
Member
There was a problem hiding this comment.
Suggested change
| - --console-plugin-images=4.0.0=$(RELATED_IMAGE_CONSOLE_PLUGIN_PF4);4.15.0=$(RELATED_IMAGE_CONSOLE_PLUGIN_PF5);4.22.0=$(RELATED_IMAGE_CONSOLE_PLUGIN) | |
| - --console-plugin-images=4.14.0=$(RELATED_IMAGE_CONSOLE_PLUGIN_PF4);4.15.0=$(RELATED_IMAGE_CONSOLE_PLUGIN_PF5);4.22.0=$(RELATED_IMAGE_CONSOLE_PLUGIN) |
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
2 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
Dependencies
n/a
Checklist
Summary by CodeRabbit
Release Notes
New Features
Refactor