🐛 Better error handling in addon adapter. #898
Conversation
Signed-off-by: Jeff Ortel <[email protected]>
Signed-off-by: Jeff Ortel <[email protected]>
WalkthroughAdds an Adapter.Wrap field, uses fmt.Errorf to convert non-error panics into errors during Run's recovery, and wires Wrap in adapter construction. Adjusts task reference handling to use local ref.ID for API calls and wraps NotFound cases with contextual ResetError messages. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Adapter
participant Handler as Task/Handler
participant WrapFn as Adapter.Wrap
participant Fmt as fmt
Caller->>Adapter: Run(task)
Adapter->>Handler: Execute task
Note over Handler,Adapter: panic occurs (r)
Handler--xAdapter: panic r
rect rgba(230,240,255,0.6)
Adapter->>Adapter: recover() -> r
alt r is error
Adapter->>WrapFn: Wrap(r, context...)
WrapFn-->>Adapter: wrapped error
else r is non-error
Adapter->>Fmt: Errorf("%#v", r)
Fmt-->>Adapter: err
Adapter->>WrapFn: Wrap(err, context...)
WrapFn-->>Adapter: wrapped error
end
end
Adapter-->>Caller: return error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Pre-merge checks (1 passed, 1 warning, 1 inconclusive)❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing Touches
🧪 Generate unit tests
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 |
jortel
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
addon/adapter.go (3)
125-133: Make recovered panic messages clearer (avoid %#v).
%#vcan add quotes or Go-syntax noise. Include the recovered value’s type and readable value.- err = fmt.Errorf("%#v", r) + err = fmt.Errorf("panic recovered: %T: %v", r, r)
7-11: Optionally capture stack trace on recovered panics.Adding a stack to the error log makes postmortems faster.
Option A (preferred): log stack
import ( - "fmt" + "fmt" + "runtime/debug" "os"- h.Log.Error(err, "Addon failed.") + if r != nil { + h.Log.Error(err, "Addon failed.", "panic", true, "stack", string(debug.Stack())) + } else { + h.Log.Error(err, "Addon failed.") + }Option B (minimal): flag that it was a panic without the stack
- h.Log.Error(err, "Addon failed.") + h.Log.Error(err, "Addon failed.", "panic", r != nil)
84-86: Exposing Wrap on Adapter—confirm intent or rename to reduce ambiguity.Having both package-level
Wrapand fieldAdapter.Wrapwith the same name can be confusing. If this is meant for addon authors, considerErrorWrapor similar; otherwise, drop it to keep the API surface small.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
addon/adapter.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
addon/adapter.go (1)
api/filter/error.go (1)
Errorf(20-23)
🔇 Additional comments (1)
addon/adapter.go (1)
170-171: LGTM: Wrap is properly wired into the adapter.No issues with the initialization.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
addon/task.go (4)
36-41: Guard missing Application ref to avoid Get(0)/Identity(0) callsIf the task has no application, this now issues Get(0) and Identity(0).List(). Please verify the client/server behavior for ID=0 and consider an explicit, contextual error instead to avoid confusing 404s or malformed paths.
ref := h.task.Application -if ref == nil { - ref = &api.Ref{} -} -r, err = h.richClient.Application.Get(ref.ID) +if ref == nil || ref.ID == 0 { + return nil, fmt.Errorf("task %d has no application reference", h.task.ID) +} +r, err = h.richClient.Application.Get(ref.ID) +if err != nil { + return nil, fmt.Errorf("get application id=%d: %w", ref.ID, err) +} @@ -identities, err := h.richClient.Application.Identity(ref.ID).List() -if err != nil { - return -} +identities, err := h.richClient.Application.Identity(ref.ID).List() +if err != nil { + return nil, fmt.Errorf("list identities for application id=%d: %w", ref.ID, err) +}Also applies to: 45-46
62-67: Similarly guard missing Platform ref to avoid Get(0)Same concern as Application(): calling Get with ID=0. Suggest explicit error for clarity and to reduce noisy downstream calls.
ref := h.task.Platform -if ref == nil { - ref = &api.Ref{} -} -r, err = h.richClient.Platform.Get(ref.ID) +if ref == nil || ref.ID == 0 { + return nil, fmt.Errorf("task %d has no platform reference", h.task.ID) +} +r, err = h.richClient.Platform.Get(ref.ID) +if err != nil { + return nil, fmt.Errorf("get platform id=%d: %w", ref.ID, err) +}
74-76: Empty addon name now flows into Get(name); verify behaviorWith the prior guard removed, name=="" will call Addon.Get(""). Please confirm this yields a clean NotFound (and not a malformed URL or unintended list call). Optionally fail fast with context.
name := h.task.Addon +r, err = nil, nil +if name == "" { + return nil, fmt.Errorf("task %d has no addon name", h.task.ID) +} r, err = h.richClient.Addon.Get(name) +if err != nil { + return nil, fmt.Errorf("get addon name=%q: %w", name, err) +}
115-115: Confirm Wrap is nil-safe or guard before callingIf Wrap(nil) is not guaranteed to return nil, guard it. If it is nil-safe, current code is fine.
-err = Wrap(err) +if err != nil { + err = Wrap(err) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
addon/task.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
addon/task.go (2)
api/application.go (1)
Application(1325-1350)api/base.go (1)
Ref(340-343)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: test-unit
Signed-off-by: Jeff Ortel <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
addon/task.go (3)
43-44: Guard against zero application ID.Avoid a pointless GET with ID=0 and return a clear error.
- r, err = h.richClient.Application.Get(ref.ID) + if ref.ID == 0 { + err = Wrap(&NotFound{ + RestError: ResetError{ + Resource: "application", + Reason: "application ID missing.", + }, + }) + return + } + r, err = h.richClient.Application.Get(ref.ID)
47-59: Preallocate identities slice to avoid repeated reallocations.Minor perf/readability improvement.
- r.Identities = []api.Ref{} - identities, err := h.richClient.Application.Identity(ref.ID).List() + identities, err := h.richClient.Application.Identity(ref.ID).List() if err != nil { return } - for _, identity := range identities { - r.Identities = append( - r.Identities, - api.Ref{ - ID: identity.ID, - Name: identity.Name, - }) - } + r.Identities = make([]api.Ref, 0, len(identities)) + for _, identity := range identities { + r.Identities = append(r.Identities, api.Ref{ + ID: identity.ID, + Name: identity.Name, + }) + }
72-73: Guard against zero platform ID.Mirror the application check to avoid ID=0 calls.
- r, err = h.richClient.Platform.Get(ref.ID) + if ref.ID == 0 { + err = Wrap(&NotFound{ + RestError: ResetError{ + Resource: "platform", + Reason: "platform ID missing.", + }, + }) + return + } + r, err = h.richClient.Platform.Get(ref.ID)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
addon/task.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
addon/task.go (5)
addon/adapter.go (6)
Application(59-59)Wrap(22-22)NotFound(56-56)ResetError(54-54)Identity(63-63)Platform(65-65)binding/application.go (1)
Application(12-14)api/error.go (1)
NotFound(84-87)api/base.go (1)
Ref(340-343)binding/identity.go (1)
Identity(8-10)
🪛 GitHub Check: vet
addon/task.go
[failure] 38-38:
github.com/konveyor/tackle2-hub/binding.NotFound struct literal uses unkeyed fields
[failure] 67-67:
github.com/konveyor/tackle2-hub/binding.NotFound struct literal uses unkeyed fields
[failure] 82-82:
github.com/konveyor/tackle2-hub/binding.NotFound struct literal uses unkeyed fields
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
Signed-off-by: Jeff Ortel <[email protected]>
jortel
changed the title
2 participants
Summary by CodeRabbit
New Features
Bug Fixes