feat: add Stop method #110
Conversation
WalkthroughThe pull request introduces updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
Client->>Server: Send request
Server-->>Client: Process request
Client->>Server: Stop request
Server-->>Client: Acknowledge stop
Client->>Server: Send request (after stop)
Server-->>Client: Reject request (no new requests accepted)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (4)
route.go (3)
188-191: LGTM! Consider adding more details to the deprecation notice.The deprecation of the
Shutdownmethod is correctly implemented, maintaining backward compatibility by calling the newStopmethod.Consider expanding the deprecation notice to include when it will be removed and why
Stopis preferred:-// DEPRECATED Use Stop instead +// DEPRECATED: Use Stop instead. This method will be removed in v2.0.0. Stop provides more flexibility with context handling.
Line range hint
193-203: LGTM! Consider adding error logging.The
Stopmethod is well-implemented, providing a clean way to gracefully shut down the server with optional context support.Consider logging the error returned by
ShutdownWithContextbefore returning it:func (r *Route) Stop(ctx ...context.Context) error { c := context.Background() if len(ctx) > 0 { c = ctx[0] } - return r.instance.ShutdownWithContext(c) + err := r.instance.ShutdownWithContext(c) + if err != nil { + // Assuming you have a logger available + // log.Error("Error shutting down server", err) + } + return err }This would provide more visibility into shutdown errors without changing the method's behavior.
Line range hint
114-150: LGTM! Consider unifying error messages for consistency.The updates to
RunTLSand the addition ofRunTLSWithCertimprove the flexibility of TLS configuration. The error checking is thorough and the implementation is clean.For consistency, consider unifying the error messages:
func (r *Route) RunTLS(host ...string) error { if len(host) == 0 { defaultHost := r.config.GetString("http.tls.host") defaultPort := r.config.GetString("http.tls.port") if defaultPort == "" { - return errors.New("port can't be empty") + return errors.New("TLS port can't be empty") } completeHost := defaultHost + ":" + defaultPort host = append(host, completeHost) } certFile := r.config.GetString("http.tls.ssl.cert") keyFile := r.config.GetString("http.tls.ssl.key") return r.RunTLSWithCert(host[0], certFile, keyFile) } // RunTLSWithCert run TLS server with cert file and key file // RunTLSWithCert 使用证书文件和密钥文件运行 TLS 服务器 func (r *Route) RunTLSWithCert(host, certFile, keyFile string) error { if host == "" { - return errors.New("host can't be empty") + return errors.New("TLS host can't be empty") } if certFile == "" || keyFile == "" { - return errors.New("certificate can't be empty") + return errors.New("TLS certificate or key can't be empty") } r.outputRoutes() color.Green().Println(termlink.Link("[HTTPS] Listening and serving HTTPS on", "https://"+host)) return r.instance.ListenTLS(host, certFile, keyFile) }This makes the error messages more specific and consistent across both methods.
route_test.go (1)
Line range hint
405-491: LGTM: TestStop function implementationThe
TestStopfunction effectively tests the behavior of theStopmethod, covering important scenarios such as rejecting new requests after stopping and processing received requests before stopping. The test cases are well-structured and provide good coverage of the functionality.Consider using a more robust synchronization mechanism
While
time.Sleepis used for synchronization, it might not be reliable in all environments or under different load conditions. Consider using a more robust synchronization mechanism, such as channels orsync.WaitGroup, to ensure the server has started before making requests and to wait for all goroutines to complete.Here's an example of how you could improve the synchronization:
go func() { + serverReady := make(chan struct{}) + go func() { + // Signal that the server is ready + serverReady <- struct{}{} assert.Nil(t, route.Run()) + }() + // Wait for the server to be ready + <-serverReady }() -time.Sleep(1 * time.Second)Enhance error handling in the setup function
The current implementation ignores the error returned by the
setupfunction. Consider handling this error explicitly to ensure that any setup failures are properly reported.Here's a suggestion to improve error handling:
-if err := test.setup(); err == nil { - assert.Nil(t, err) +if err := test.setup(); err != nil { + t.Fatalf("Test setup failed: %v", err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
- go.mod (3 hunks)
- route.go (1 hunks)
- route_test.go (3 hunks)
🔇 Additional comments (9)
route.go (1)
Line range hint
1-238: Overall, the changes look good and improve the codebase.The updates to the
Routestruct and its methods enhance the server's shutdown process and TLS configuration flexibility. The code maintains backward compatibility while introducing new, more flexible methods. The minor suggestions provided will further improve code consistency and error handling.go.mod (7)
23-23: Approve the Google Cloud compute metadata update and review new features.The
cloud.google.com/go/compute/metadatadependency has been updated from v0.3.0 to v0.5.0. This minor version update likely includes new features and improvements.It's recommended to review the changelog for any new features or improvements that could benefit your project. You can find the changelog at: https://github.com/googleapis/google-cloud-go/blob/main/compute/metadata/CHANGES.md
154-154: Approve the OAuth2 library update and review new features.The
golang.org/x/oauth2dependency has been updated from v0.21.0 to v0.22.0. This minor version update likely includes new features and improvements.It's recommended to review the changelog for any new features or improvements that could benefit your project. You can find the release notes at: https://github.com/golang/oauth2/releases
163-163: Approve the Google RPC proto update.The
google.golang.org/genproto/googleapis/rpcdependency has been updated to a more recent commit (dated 2024-08-14), consistent with the previous googleapis/api update.This update aligns with the googleapis/api update, which is a good practice for maintaining consistency across related Google API dependencies.
164-164: Approve the gRPC update and review new features.The
google.golang.org/grpcdependency has been updated from v1.66.2 to v1.67.0. This minor version update likely includes new features and improvements.It's recommended to review the changelog for any new features or improvements that could benefit your project. You can find the release notes at: https://github.com/grpc/grpc-go/releases/tag/v1.67.0
Line range hint
1-180: Summary of dependency updatesThis update includes several minor version bumps and commit-specific updates to various dependencies. These changes are likely to include bug fixes, performance improvements, and new features. Here's a summary of the key updates:
github.com/goravel/framework: Updated to a newer development version.cloud.google.com/go/compute/metadata: Minor version update (v0.3.0 to v0.5.0).golang.org/x/oauth2: Minor version update (v0.21.0 to v0.22.0).google.golang.org/genproto/googleapis/apiandgoogle.golang.org/genproto/googleapis/rpc: Updated to a more recent commit.google.golang.org/grpc: Minor version update (v1.66.2 to v1.67.0).These updates represent good maintenance practices. However, it's important to:
- Review the changelogs of updated dependencies for any new features or breaking changes.
- Run your test suite to ensure compatibility with the updated dependencies.
- Monitor your application's performance after deploying these changes to production.
162-162: Approve the Google API proto update and verify stability.The
google.golang.org/genproto/googleapis/apidependency has been updated to a more recent commit (dated 2024-08-14).As this is a commit-specific update rather than a semantic version, please ensure that this version is stable and compatible with your project. Run the following command to check for any significant changes:
#!/bin/bash # Description: Check for significant changes in the Google API proto update. # Test: Compare the old and new versions go mod download google.golang.org/genproto/googleapis/[email protected] go mod download google.golang.org/genproto/googleapis/[email protected] # List changes between the two versions git diff --no-index $(go env GOPATH)/pkg/mod/google.golang.org/genproto/googleapis/[email protected] $(go env GOPATH)/pkg/mod/google.golang.org/genproto/googleapis/[email protected]Review the output for any breaking changes or significant modifications that might affect your codebase.
11-11: Approve the framework update and verify compatibility.The
github.com/goravel/frameworkdependency has been updated to a newer development version. This update is likely to include bug fixes and minor improvements.Please ensure that this update is compatible with your current implementation. Run the following command to check for any breaking changes or deprecations:
Review the output for any breaking changes or deprecations that might affect your codebase.
route_test.go (1)
431-431: Consistent use of the Stop methodThe change from
ShutdowntoStopis correctly implemented in the test cases. This renaming is consistent with the changes mentioned in the summary and maintains the expected functionality while using a more intuitive method name.Also applies to: 456-456
📑 Description
Summary by CodeRabbit
New Features
Stopmethod for gracefully shutting down the server, replacing the deprecatedShutdownmethod.RunTLSWithCertmethod for running a TLS server with specified certificate and key files.Bug Fixes
Documentation
Tests
ShutdowntoStopand ensure proper functionality with the new methods.✅ Checks