Update Plugin API with optional New Resources#140
Merged
Conversation
Update of `crane-lib` to allow transform plugins to generate entirely new Kubernetes resources. The implementation ensures full backward compatibility without the need for a V2 API. **Core Architecture Updates** * **Extended API:** Both `PluginResponse` and `RunnerResponse` now include a `NewResources` field to hold standard Kubernetes unstructured objects. * **Graceful Degradation:** Relies on the JSON `omitempty` tag. Older plugins simply omit this field, which automatically resolves to an empty list without breaking the execution. * **Simplified Runner Logic:** The runner iterates through plugins and aggregates any `NewResources` provided. It avoids complex version validation, relying solely on whether the new resources array contains items. Related to migtools/crane#415 Fixes migtools#139 Signed-off-by: Marek Aufart <maufart@redhat.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a NewResources field to PluginResponse and RunnerResponse, a NewRunner constructor, aggregates plugin-emitted new unstructured Kubernetes resources in Runner.Run, and extends tests to validate multiple generation and backward-compatibility scenarios. ChangesPlugin-generated new resources API extension
Sequence DiagramsequenceDiagram
participant Runner
participant Plugin
participant Accumulator as newResources_Accumulator
participant Response as RunnerResponse
Runner->>Plugin: Execute plugin.Run
Plugin-->>Runner: return PluginResponse (NewResources, Patches, IsWhiteOut, Version)
Runner->>Accumulator: append PluginResponse.NewResources
Runner->>Runner: debug log (plugin name, appended count)
Accumulator-->>Response: aggregated NewResources
Runner-->>Response: return RunnerResponse (NewResources, other fields)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@transform/runner.go`:
- Around line 87-91: The call r.Log.Debugf(...) can panic if Runner.Log is nil
when a plugin returns NewResources; update the block handling resp.NewResources
in function/method that appends to newResources to first check for r.Log != nil
before calling Debugf (or use a nil-safe logger accessor), e.g. wrap the
r.Log.Debugf call in an if r.Log != nil { ... } guard and keep appending
resp.NewResources to newResources and using plugin.Metadata().Name and
len(resp.NewResources) only inside that guarded section so no nil dereference
occurs when Runner.Log is unset.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e62372c6-7321-4bda-9150-3773adcf9c4d
📒 Files selected for processing (3)
transform/plugin.gotransform/runner.gotransform/runner_test.go
Signed-off-by: Marek Aufart <maufart@redhat.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
transform/runner.go (1)
25-30:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle nil logger input in
NewRunner.
NewRunneracceptslogger *logrus.Loggerbut does not guardnil. A nil input leavesRunner.Lognil and can still panic on laterDebugfcalls.Suggested fix
func NewRunner(logger *logrus.Logger, pluginPriorities map[string]int, optionalFlags map[string]string) *Runner { + if logger == nil { + logger = logrus.New() + } return &Runner{ Log: logger, PluginPriorities: pluginPriorities, OptionalFlags: optionalFlags, } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transform/runner.go` around lines 25 - 30, NewRunner currently assigns the passed logger pointer directly to Runner.Log without guarding against nil, which can cause panics on later calls like Runner.Log.Debugf; update NewRunner to check if the supplied logger is nil and if so create and assign a default logger (e.g., by calling logrus.New()) before returning the &Runner, keeping the existing fields PluginPriorities and OptionalFlags unchanged so all callers (and methods that call Debugf/Infof) always have a non-nil Log to use.
🧹 Nitpick comments (1)
transform/runner_test.go (1)
552-568: ⚡ Quick winAdd a nil-logger constructor test case.
TestNewRunneronly validates non-nil input. Add a case forNewRunner(nil, ...)to prevent regressions on logger safety.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transform/runner_test.go` around lines 552 - 568, TestNewRunner lacks a case that passes a nil logger to NewRunner; add a subtest or additional assertions that call NewRunner(nil, priorities, flags) and assert the returned runner has a non-nil Log (i.e., NewRunner must guard against a nil logger), and still correctly sets PluginPriorities and OptionalFlags. Locate NewRunner and TestNewRunner in runner_test.go, create the nil-logger invocation, and verify runner.Log != nil and runner.PluginPriorities["plugin1"] == 1 and runner.OptionalFlags["flag1"] == "value1".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@transform/runner.go`:
- Around line 25-30: NewRunner currently assigns the passed logger pointer
directly to Runner.Log without guarding against nil, which can cause panics on
later calls like Runner.Log.Debugf; update NewRunner to check if the supplied
logger is nil and if so create and assign a default logger (e.g., by calling
logrus.New()) before returning the &Runner, keeping the existing fields
PluginPriorities and OptionalFlags unchanged so all callers (and methods that
call Debugf/Infof) always have a non-nil Log to use.
---
Nitpick comments:
In `@transform/runner_test.go`:
- Around line 552-568: TestNewRunner lacks a case that passes a nil logger to
NewRunner; add a subtest or additional assertions that call NewRunner(nil,
priorities, flags) and assert the returned runner has a non-nil Log (i.e.,
NewRunner must guard against a nil logger), and still correctly sets
PluginPriorities and OptionalFlags. Locate NewRunner and TestNewRunner in
runner_test.go, create the nil-logger invocation, and verify runner.Log != nil
and runner.PluginPriorities["plugin1"] == 1 and runner.OptionalFlags["flag1"] ==
"value1".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 810a7a79-75f7-4ba8-9854-13dbcdc33364
📒 Files selected for processing (2)
transform/runner.gotransform/runner_test.go
Signed-off-by: Marek Aufart <maufart@redhat.com>
Signed-off-by: Marek Aufart <maufart@redhat.com>
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
4 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.
Update of
crane-libto allow transform plugins to generate entirely new Kubernetes resources. The implementation ensures full backward compatibility without the need for a V2 API.Core Architecture Updates
PluginResponseandRunnerResponsenow include aNewResourcesfield to hold standard Kubernetes unstructured objects.omitemptytag. Older plugins simply omit this field, which automatically resolves to an empty list without breaking the execution.NewResourcesprovided. It avoids complex version validation, relying solely on whether the new resources array contains items.Related to migtools/crane#415
Fixes #139
Summary by CodeRabbit
New Features
Tests