fix: reuse template for oci plugins tests#2613
Merged
Conversation
SkArchon
requested review from
Noroth,
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
Contributor
WalkthroughReplaces an OCI-specific embedded plugin JSON template with a general plugins template in router tests and injects OCI image references at runtime via a new addOCIImageReferences function; removes the embedded OCI plugins template from the test environment. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 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
Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
![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)
router-tests/router_oci_plugin_test.go (1)
116-135: Centralize the OCI image map and fail fast on template drift.This helper duplicates repo/tag literals already used by the test setup, and it silently ignores any new plugin datasource added to
ConfigWithPluginsJSONTemplate. A small lookup table plus an explicit failure on unexpected plugin names will make future template changes much easier to catch.♻️ Proposed refactor
+var ociPluginRepositories = map[string]string{ + "projects": "test-org/projects", + "courses": "test-org/courses", +} + +const ociPluginReference = "v1" + func addOCIImageReferences(routerConfig *nodev1.RouterConfig) { for _, ds := range routerConfig.EngineConfig.DatasourceConfigurations { plugin := ds.GetCustomGraphql().GetGrpc().GetPlugin() if plugin == nil { continue } - switch plugin.Name { - case "projects": - plugin.ImageReference = &nodev1.ImageReference{ - Repository: "test-org/projects", - Reference: "v1", - } - case "courses": - plugin.ImageReference = &nodev1.ImageReference{ - Repository: "test-org/courses", - Reference: "v1", - } + repository, ok := ociPluginRepositories[plugin.Name] + if !ok { + panic(fmt.Sprintf("unexpected plugin in OCI test template: %s", plugin.Name)) + } + plugin.ImageReference = &nodev1.ImageReference{ + Repository: repository, + Reference: ociPluginReference, + } } } }Also worth reusing the same constants/map in the
buildAndPushPluginImagecalls so the push side and pull side can't drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/router_oci_plugin_test.go` around lines 116 - 135, The helper addOCIImageReferences duplicates hardcoded repo/tag literals and ignores unknown plugins; create a single lookup map (e.g., map[string]nodev1.ImageReference) that maps plugin names to their ImageReference and replace the switch in addOCIImageReferences to look up entries in that map, returning or calling t.Fatalf (fail fast) if a plugin name is not found (to detect template drift relative to ConfigWithPluginsJSONTemplate); also reuse the same lookup constants/map in buildAndPushPluginImage calls so the image push and test-time image references are sourced from the same canonical data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@router-tests/router_oci_plugin_test.go`:
- Around line 116-135: The helper addOCIImageReferences duplicates hardcoded
repo/tag literals and ignores unknown plugins; create a single lookup map (e.g.,
map[string]nodev1.ImageReference) that maps plugin names to their ImageReference
and replace the switch in addOCIImageReferences to look up entries in that map,
returning or calling t.Fatalf (fail fast) if a plugin name is not found (to
detect template drift relative to ConfigWithPluginsJSONTemplate); also reuse the
same lookup constants/map in buildAndPushPluginImage calls so the image push and
test-time image references are sourced from the same canonical data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 955c3db5-6668-43ea-b781-07eec4122ecc
📒 Files selected for processing (3)
router-tests/router_oci_plugin_test.gorouter-tests/testenv/testdata/configWithOCIPlugins.jsonrouter-tests/testenv/testenv.go
💤 Files with no reviewable changes (1)
- router-tests/testenv/testenv.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
router-tests/router_oci_plugin_test.go (1)
116-135: Make the modifier fail fast on template drift.Line 117 dereferences
routerConfig.EngineConfigdirectly, and the helper silently does nothing if either expected plugin name disappears from the shared template. Using generated getters and asserting both rewrites happened gives a much clearer failure than a nil panic or an indirect startup error.Suggested hardening
func addOCIImageReferences(routerConfig *nodev1.RouterConfig) { - for _, ds := range routerConfig.EngineConfig.DatasourceConfigurations { + matched := map[string]bool{ + "projects": false, + "courses": false, + } + + for _, ds := range routerConfig.GetEngineConfig().GetDatasourceConfigurations() { plugin := ds.GetCustomGraphql().GetGrpc().GetPlugin() if plugin == nil { continue } switch plugin.Name { case "projects": + matched["projects"] = true plugin.ImageReference = &nodev1.ImageReference{ Repository: "test-org/projects", Reference: "v1", } case "courses": + matched["courses"] = true plugin.ImageReference = &nodev1.ImageReference{ Repository: "test-org/courses", Reference: "v1", } } } + + if !matched["projects"] || !matched["courses"] { + panic("expected projects and courses plugins in ConfigWithPluginsJSONTemplate") + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/router_oci_plugin_test.go` around lines 116 - 135, The helper addOCIImageReferences currently dereferences routerConfig.EngineConfig and silently returns if plugins are missing; update it to fail fast by (1) using the generated getter routerConfig.GetEngineConfig() and guarding against nil, (2) iterating datasource configurations via GetDatasourceConfigurations(), checking each plugin via GetCustomGraphql().GetGrpc().GetPlugin(), setting ImageReference on matches for "projects" and "courses", and (3) verifying both rewrites occurred (e.g. track booleans for projects/courses) and return an error or call t.Fatalf/panic when either expected plugin is absent or not rewritten so the test fails immediately instead of causing a nil panic or indirect startup error. Ensure references to addOCIImageReferences, routerConfig.GetEngineConfig, GetDatasourceConfigurations, GetCustomGraphql, GetGrpc, GetPlugin, plugin.Name, and plugin.ImageReference are used to locate and implement the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@router-tests/router_oci_plugin_test.go`:
- Around line 116-135: The helper addOCIImageReferences currently dereferences
routerConfig.EngineConfig and silently returns if plugins are missing; update it
to fail fast by (1) using the generated getter routerConfig.GetEngineConfig()
and guarding against nil, (2) iterating datasource configurations via
GetDatasourceConfigurations(), checking each plugin via
GetCustomGraphql().GetGrpc().GetPlugin(), setting ImageReference on matches for
"projects" and "courses", and (3) verifying both rewrites occurred (e.g. track
booleans for projects/courses) and return an error or call t.Fatalf/panic when
either expected plugin is absent or not rewritten so the test fails immediately
instead of causing a nil panic or indirect startup error. Ensure references to
addOCIImageReferences, routerConfig.GetEngineConfig,
GetDatasourceConfigurations, GetCustomGraphql, GetGrpc, GetPlugin, plugin.Name,
and plugin.ImageReference are used to locate and implement the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 63ac4d46-4807-4c2a-96f7-de7752cdc125
📒 Files selected for processing (1)
router-tests/router_oci_plugin_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2613 +/- ##
==========================================
- Coverage 64.25% 62.39% -1.87%
==========================================
Files 301 244 -57
Lines 42658 25775 -16883
Branches 4558 0 -4558
==========================================
- Hits 27410 16082 -11328
+ Misses 15227 8318 -6909
- Partials 21 1375 +1354 🚀 New features to boost your workflow:
|
devsergiy
approved these changes
Mar 9, 2026
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.
This PR reuses the OCI plugins template for OCI integration tests.
Summary by CodeRabbit
Checklist