Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the lifecycle package to embed FuncService within both Lifecycle and SignalService types, creating a more consistent service architecture. The changes also standardize error handling by wrapping context errors with stack traces throughout the codebase.
- Embed
FuncServiceinLifecycleandSignalServicestructs to provide consistent service behavior - Add interface compliance assertions and method overrides for embedded service functionality
- Standardize error handling by wrapping context errors with stack traces using
errors.WithStack
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| lifecycle/lifecycle.go | Embeds FuncService in Lifecycle struct and adds service interface methods |
| lifecycle/signal.go | Adds service interface methods to SignalService and wraps context errors |
| lifecycle/func.go | Adds interface compliance assertions and wraps context errors with stack traces |
| lifecycle/func_test.go | Updates test code to wrap context errors with stack traces |
| lifecycle/lifecycle_test.go | Changes error equality checks to use ErrorIs for wrapped errors |
| .github/workflows/go.yml | Adds GitHub Actions workflow for CI/CD pipeline |
|
|
||
| // WithStop is not supported for SignalService. | ||
| func (s *SignalService) WithStop(stop func(ctx context.Context) error) *SignalService { | ||
| panic("this method is not supported for SignalService") |
There was a problem hiding this comment.
The panic message should be more descriptive and follow Go conventions. Consider: "SignalService does not support WithStop method" or "WithStop is not supported on SignalService".
| panic("this method is not supported for SignalService") | |
| panic("SignalService does not support WithStop method") |
| func (lc *Lifecycle) WithStopTimeout(timeout time.Duration) *Lifecycle { | ||
| // WithStop is not supported for Lifecycle. | ||
| func (lc *Lifecycle) WithStop(stop func(ctx context.Context) error) *Lifecycle { | ||
| panic("this method is not supported for Lifecycle") |
There was a problem hiding this comment.
The panic message should be more descriptive and follow Go conventions. Consider: "Lifecycle does not support WithStop method" or "WithStop is not supported on Lifecycle".
| panic("this method is not supported for Lifecycle") | |
| panic("WithStop is not supported on Lifecycle") |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Type of change
Description
Related issues
Notes for reviewer