🔥 feat: Add Support for service dependencies #3434
Conversation
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
WalkthroughThis update introduces a new "Service" interface for managing external dependencies in the application lifecycle, along with related configuration options, methods for starting and shutting down services, and comprehensive documentation and tests. The App struct and configuration are extended to support service management, and startup/shutdown flows are updated to automatically handle configured services. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant App as App
participant Service as Service(s)
participant Cfg as Config
Dev->>Cfg: Define Services, Startup/Shutdown Context Providers
Dev->>App: New(Config)
App->>App: Set defaults, initialize fields
App->>App: hasServices()
alt Services configured
App->>App: servicesStartupCtx()
App->>Service: Start(ctx)
Service-->>App: (success or error)
App->>App: Track started services
end
App->>App: Register post-shutdown hook
App->>Service: Terminate(ctx) (on shutdown)
Service-->>App: (success or error)
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (7)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3434 +/- ##
==========================================
+ Coverage 84.20% 84.35% +0.15%
==========================================
Files 119 120 +1
Lines 11947 12083 +136
==========================================
+ Hits 10060 10193 +133
- Misses 1460 1462 +2
- Partials 427 428 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
devtime_dependencies_test.go (3)
135-152: Context cancellation test could be more robust.The current test uses an extremely short timeout (1 nanosecond) which might be flaky. Additionally, we should verify if the dependency was actually started before context cancellation.
Consider using a more realistic timeout and checking the dependency's state:
- ctx, cancel := context.WithTimeout(context.Background(), 1*time.Nanosecond) + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Millisecond) defer cancel() // Start dependencies with canceled context err := app.startDevTimeDependencies(ctx) require.ErrorIs(t, err, context.DeadlineExceeded) + require.False(t, slowDep.started, "Dependency should not have been started due to context cancellation")Also, "cancelled" is the British English spelling. The standard Go library uses "canceled" (American English).
🧰 Tools
🪛 GitHub Check: lint
[failure] 149-149:
cancelledis a misspelling ofcanceled(misspell)
[failure] 145-145:
cancelledis a misspelling ofcanceled(misspell)
154-175: Verify dependency state after termination attempt.Similar to the start test, this test should verify that the dependency's terminated flag is set to false when context is canceled.
// Shutdown dependencies with canceled context err = app.shutdownDevTimeDependencies(ctx) require.ErrorIs(t, err, context.DeadlineExceeded) + require.False(t, slowDep.terminated, "Dependency should not have been terminated due to context cancellation")🧰 Tools
🪛 GitHub Check: lint
[failure] 164-164:
cancelledis a misspelling ofcanceled(misspell)
177-198: Consider verifying dependency states after errors.The test confirms that errors are properly aggregated, but doesn't verify the state of each dependency after the operation.
Add verification of dependency states:
// Test start errors err := app.startDevTimeDependencies(context.Background()) require.Error(t, err) // Verify error message contains both error messages errMsg := err.Error() require.Contains(t, errMsg, "start error 1") require.Contains(t, errMsg, "start error 2") + // Verify all dependencies were attempted to be started + for _, dep := range deps { + mockDep := dep.(*mockDependency) + require.True(t, mockDep.started, "Dependency should have been started despite errors") + }devtime_dependencies.go (1)
27-75: Consider using error wrapping consistentlyThe error handling pattern is good, but notice the inconsistency between wrapping errors in
startDevTimeDependenciesversusshutdownDevTimeDependencies.For consistency, consider using the same format for both functions. For example:
- errs = append(errs, fmt.Errorf("context canceled: %w", err)) + errs = append(errs, fmt.Errorf("dependency %s terminate: %w", "unknown", err))This maintains consistent error message formatting throughout the codebase.
🧰 Tools
🪛 GitHub Check: lint
[failure] 60-60:
cancelledis a misspelling ofcanceled(misspell)
[failure] 59-59:
cancelledis a misspelling ofcanceled(misspell)
[failure] 36-36:
cancelledis a misspelling ofcanceled(misspell)
[failure] 34-34:
cancelledis a misspelling ofcanceled(misspell)🪛 GitHub Actions: golangci-lint
[error] 34-34:
cancelledis a misspelling ofcanceled(misspell)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app.go(2 hunks)devtime_dependencies.go(1 hunks)devtime_dependencies_test.go(1 hunks)docs/api/constants.md(1 hunks)docs/api/devtime_dependencies.md(1 hunks)docs/whats_new.md(2 hunks)listen.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
app.go (3)
devtime_dependencies.go (1)
DevTimeDependency(10-20)log/fiberlog.go (1)
Warnf(54-56)hooks.go (1)
Hooks(21-35)
🪛 markdownlint-cli2 (0.17.2)
docs/whats_new.md
810-810: Hard tabs
Column: 1
(MD010, no-hard-tabs)
816-816: Hard tabs
Column: 1
(MD010, no-hard-tabs)
817-817: Hard tabs
Column: 1
(MD010, no-hard-tabs)
823-823: Hard tabs
Column: 1
(MD010, no-hard-tabs)
828-828: Hard tabs
Column: 1
(MD010, no-hard-tabs)
829-829: Hard tabs
Column: 1
(MD010, no-hard-tabs)
835-835: Hard tabs
Column: 1
(MD010, no-hard-tabs)
836-836: Hard tabs
Column: 1
(MD010, no-hard-tabs)
838-838: Hard tabs
Column: 1
(MD010, no-hard-tabs)
docs/api/devtime_dependencies.md
70-70: Hard tabs
Column: 1
(MD010, no-hard-tabs)
71-71: Hard tabs
Column: 1
(MD010, no-hard-tabs)
73-73: Hard tabs
Column: 1
(MD010, no-hard-tabs)
74-74: Hard tabs
Column: 1
(MD010, no-hard-tabs)
75-75: Hard tabs
Column: 1
(MD010, no-hard-tabs)
79-79: Hard tabs
Column: 1
(MD010, no-hard-tabs)
84-84: Hard tabs
Column: 1
(MD010, no-hard-tabs)
85-85: Hard tabs
Column: 1
(MD010, no-hard-tabs)
86-86: Hard tabs
Column: 1
(MD010, no-hard-tabs)
87-87: Hard tabs
Column: 1
(MD010, no-hard-tabs)
88-88: Hard tabs
Column: 1
(MD010, no-hard-tabs)
90-90: Hard tabs
Column: 1
(MD010, no-hard-tabs)
91-91: Hard tabs
Column: 1
(MD010, no-hard-tabs)
97-97: Hard tabs
Column: 1
(MD010, no-hard-tabs)
102-102: Hard tabs
Column: 1
(MD010, no-hard-tabs)
103-103: Hard tabs
Column: 1
(MD010, no-hard-tabs)
107-107: Hard tabs
Column: 1
(MD010, no-hard-tabs)
109-109: Hard tabs
Column: 1
(MD010, no-hard-tabs)
110-110: Hard tabs
Column: 1
(MD010, no-hard-tabs)
111-111: Hard tabs
Column: 1
(MD010, no-hard-tabs)
113-113: Hard tabs
Column: 1
(MD010, no-hard-tabs)
115-115: Hard tabs
Column: 1
(MD010, no-hard-tabs)
117-117: Hard tabs
Column: 1
(MD010, no-hard-tabs)
118-118: Hard tabs
Column: 1
(MD010, no-hard-tabs)
119-119: Hard tabs
Column: 1
(MD010, no-hard-tabs)
120-120: Hard tabs
Column: 1
(MD010, no-hard-tabs)
121-121: Hard tabs
Column: 1
(MD010, no-hard-tabs)
122-122: Hard tabs
Column: 1
(MD010, no-hard-tabs)
124-124: Hard tabs
Column: 1
(MD010, no-hard-tabs)
125-125: Hard tabs
Column: 1
(MD010, no-hard-tabs)
126-126: Hard tabs
Column: 1
(MD010, no-hard-tabs)
127-127: Hard tabs
Column: 1
(MD010, no-hard-tabs)
128-128: Hard tabs
Column: 1
(MD010, no-hard-tabs)
129-129: Hard tabs
Column: 1
(MD010, no-hard-tabs)
131-131: Hard tabs
Column: 1
(MD010, no-hard-tabs)
132-132: Hard tabs
Column: 1
(MD010, no-hard-tabs)
134-134: Hard tabs
Column: 1
(MD010, no-hard-tabs)
135-135: Hard tabs
Column: 1
(MD010, no-hard-tabs)
136-136: Hard tabs
Column: 1
(MD010, no-hard-tabs)
137-137: Hard tabs
Column: 1
(MD010, no-hard-tabs)
139-139: Hard tabs
Column: 1
(MD010, no-hard-tabs)
152-152: Hard tabs
Column: 1
(MD010, no-hard-tabs)
153-153: Hard tabs
Column: 1
(MD010, no-hard-tabs)
154-154: Hard tabs
Column: 1
(MD010, no-hard-tabs)
156-156: Hard tabs
Column: 1
(MD010, no-hard-tabs)
157-157: Hard tabs
Column: 1
(MD010, no-hard-tabs)
158-158: Hard tabs
Column: 1
(MD010, no-hard-tabs)
162-162: Hard tabs
Column: 1
(MD010, no-hard-tabs)
163-163: Hard tabs
Column: 1
(MD010, no-hard-tabs)
164-164: Hard tabs
Column: 1
(MD010, no-hard-tabs)
168-168: Hard tabs
Column: 1
(MD010, no-hard-tabs)
173-173: Hard tabs
Column: 1
(MD010, no-hard-tabs)
174-174: Hard tabs
Column: 1
(MD010, no-hard-tabs)
175-175: Hard tabs
Column: 1
(MD010, no-hard-tabs)
176-176: Hard tabs
Column: 1
(MD010, no-hard-tabs)
177-177: Hard tabs
Column: 1
(MD010, no-hard-tabs)
179-179: Hard tabs
Column: 1
(MD010, no-hard-tabs)
180-180: Hard tabs
Column: 1
(MD010, no-hard-tabs)
186-186: Hard tabs
Column: 1
(MD010, no-hard-tabs)
191-191: Hard tabs
Column: 1
(MD010, no-hard-tabs)
192-192: Hard tabs
Column: 1
(MD010, no-hard-tabs)
196-196: Hard tabs
Column: 1
(MD010, no-hard-tabs)
198-198: Hard tabs
Column: 1
(MD010, no-hard-tabs)
199-199: Hard tabs
Column: 1
(MD010, no-hard-tabs)
200-200: Hard tabs
Column: 1
(MD010, no-hard-tabs)
202-202: Hard tabs
Column: 1
(MD010, no-hard-tabs)
204-204: Hard tabs
Column: 1
(MD010, no-hard-tabs)
206-206: Hard tabs
Column: 1
(MD010, no-hard-tabs)
207-207: Hard tabs
Column: 1
(MD010, no-hard-tabs)
208-208: Hard tabs
Column: 1
(MD010, no-hard-tabs)
209-209: Hard tabs
Column: 1
(MD010, no-hard-tabs)
210-210: Hard tabs
Column: 1
(MD010, no-hard-tabs)
211-211: Hard tabs
Column: 1
(MD010, no-hard-tabs)
213-213: Hard tabs
Column: 1
(MD010, no-hard-tabs)
214-214: Hard tabs
Column: 1
(MD010, no-hard-tabs)
215-215: Hard tabs
Column: 1
(MD010, no-hard-tabs)
216-216: Hard tabs
Column: 1
(MD010, no-hard-tabs)
217-217: Hard tabs
Column: 1
(MD010, no-hard-tabs)
218-218: Hard tabs
Column: 1
(MD010, no-hard-tabs)
220-220: Hard tabs
Column: 1
(MD010, no-hard-tabs)
221-221: Hard tabs
Column: 1
(MD010, no-hard-tabs)
223-223: Hard tabs
Column: 1
(MD010, no-hard-tabs)
224-224: Hard tabs
Column: 1
(MD010, no-hard-tabs)
225-225: Hard tabs
Column: 1
(MD010, no-hard-tabs)
226-226: Hard tabs
Column: 1
(MD010, no-hard-tabs)
228-228: Hard tabs
Column: 1
(MD010, no-hard-tabs)
229-229: Hard tabs
Column: 1
(MD010, no-hard-tabs)
231-231: Hard tabs
Column: 1
(MD010, no-hard-tabs)
232-232: Hard tabs
Column: 1
(MD010, no-hard-tabs)
233-233: Hard tabs
Column: 1
(MD010, no-hard-tabs)
234-234: Hard tabs
Column: 1
(MD010, no-hard-tabs)
235-235: Hard tabs
Column: 1
(MD010, no-hard-tabs)
237-237: Hard tabs
Column: 1
(MD010, no-hard-tabs)
238-238: Hard tabs
Column: 1
(MD010, no-hard-tabs)
239-239: Hard tabs
Column: 1
(MD010, no-hard-tabs)
240-240: Hard tabs
Column: 1
(MD010, no-hard-tabs)
241-241: Hard tabs
Column: 1
(MD010, no-hard-tabs)
243-243: Hard tabs
Column: 1
(MD010, no-hard-tabs)
244-244: Hard tabs
Column: 1
(MD010, no-hard-tabs)
245-245: Hard tabs
Column: 1
(MD010, no-hard-tabs)
246-246: Hard tabs
Column: 1
(MD010, no-hard-tabs)
247-247: Hard tabs
Column: 1
(MD010, no-hard-tabs)
248-248: Hard tabs
Column: 1
(MD010, no-hard-tabs)
250-250: Hard tabs
Column: 1
(MD010, no-hard-tabs)
251-251: Hard tabs
Column: 1
(MD010, no-hard-tabs)
253-253: Hard tabs
Column: 1
(MD010, no-hard-tabs)
254-254: Hard tabs
Column: 1
(MD010, no-hard-tabs)
256-256: Hard tabs
Column: 1
(MD010, no-hard-tabs)
257-257: Hard tabs
Column: 1
(MD010, no-hard-tabs)
258-258: Hard tabs
Column: 1
(MD010, no-hard-tabs)
259-259: Hard tabs
Column: 1
(MD010, no-hard-tabs)
261-261: Hard tabs
Column: 1
(MD010, no-hard-tabs)
262-262: Hard tabs
Column: 1
(MD010, no-hard-tabs)
263-263: Hard tabs
Column: 1
(MD010, no-hard-tabs)
264-264: Hard tabs
Column: 1
(MD010, no-hard-tabs)
265-265: Hard tabs
Column: 1
(MD010, no-hard-tabs)
266-266: Hard tabs
Column: 1
(MD010, no-hard-tabs)
267-267: Hard tabs
Column: 1
(MD010, no-hard-tabs)
269-269: Hard tabs
Column: 1
(MD010, no-hard-tabs)
270-270: Hard tabs
Column: 1
(MD010, no-hard-tabs)
272-272: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🪛 GitHub Check: lint
devtime_dependencies_test.go
[failure] 164-164:
cancelled is a misspelling of canceled (misspell)
[failure] 149-149:
cancelled is a misspelling of canceled (misspell)
[failure] 145-145:
cancelled is a misspelling of canceled (misspell)
[failure] 30-30:
unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
[failure] 21-21:
unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
[failure] 13-13:
fieldalignment: struct with 48 pointer bytes could be 40 (govet)
devtime_dependencies.go
[failure] 60-60:
cancelled is a misspelling of canceled (misspell)
[failure] 59-59:
cancelled is a misspelling of canceled (misspell)
[failure] 36-36:
cancelled is a misspelling of canceled (misspell)
[failure] 34-34:
cancelled is a misspelling of canceled (misspell)
🪛 GitHub Actions: golangci-lint
devtime_dependencies.go
[error] 34-34: cancelled is a misspelling of canceled (misspell)
🪛 GitHub Check: markdownlint
docs/api/devtime_dependencies.md
[failure] 86-86: Hard tabs
docs/api/devtime_dependencies.md:86:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 85-85: Hard tabs
docs/api/devtime_dependencies.md:85:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 84-84: Hard tabs
docs/api/devtime_dependencies.md:84:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 79-79: Hard tabs
docs/api/devtime_dependencies.md:79:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 75-75: Hard tabs
docs/api/devtime_dependencies.md:75:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 74-74: Hard tabs
docs/api/devtime_dependencies.md:74:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 73-73: Hard tabs
docs/api/devtime_dependencies.md:73:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 71-71: Hard tabs
docs/api/devtime_dependencies.md:71:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 70-70: Hard tabs
docs/api/devtime_dependencies.md:70:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 9-9: Multiple consecutive blank lines
docs/api/devtime_dependencies.md:9 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md012.md
🪛 GitHub Actions: markdownlint
docs/api/devtime_dependencies.md
[error] 9-9: markdownlint MD012/no-multiple-blanks: Multiple consecutive blank lines [Expected: 1; Actual: 2]
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Compare
- GitHub Check: Analyse
- GitHub Check: unit (1.23.x, macos-13)
- GitHub Check: unit (1.24.x, macos-latest)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: repeated
🔇 Additional comments (10)
docs/api/constants.md (1)
5-5: Update sidebar_position to accommodate new documentation
Thesidebar_positionvalue has been incremented from 9 to 10 to ensure this Constants page now sits in the correct place in the sidebar after the newly added DevTimeDependencies docs.listen.go (1)
386-393: Good implementation of dev-time dependencies display in startup log!This addition nicely follows the existing pattern for displaying startup information, using consistent formatting and color schemes. The dependency count and individual entries with checkmarks provide clear visibility of active development dependencies.
app.go (2)
414-417: Well-documented configuration field for development dependencies.The documentation clearly indicates this is an optional field with a sensible default. This addition aligns with Fiber's pattern of providing flexible configuration options.
613-626: Verify context handling for long-running dependencies.The implementation correctly starts dependencies at app creation and registers a shutdown hook, but using a background context without timeout could potentially cause issues with dependencies that hang during startup or shutdown.
Consider whether timeouts should be applied to prevent potential resource leaks:
-ctx := context.Background() +ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) +defer cancel()The error handling with log warnings is appropriate for informational purposes while allowing the application to continue functioning.
docs/whats_new.md (1)
848-868: Great output example.The console output example clearly shows how the dependencies appear in the server startup logs, helping users understand what to expect when implementing this feature.
devtime_dependencies.go (3)
9-20: Well-designed interface with clear lifecycle methodsThe
DevTimeDependencyinterface provides a clean and concise API with three essential methods that cover the complete lifecycle of a development-time dependency.
22-25: LGTM: Simple helper functionThis helper function clearly encapsulates the check for configured dependencies.
29-50: Good error handling pattern for context cancellationThe implementation correctly handles context cancellation by checking for errors early and returning immediately, which allows the user to respond appropriately.
🧰 Tools
🪛 GitHub Check: lint
[failure] 36-36:
cancelledis a misspelling ofcanceled(misspell)
[failure] 34-34:
cancelledis a misspelling ofcanceled(misspell)🪛 GitHub Actions: golangci-lint
[error] 34-34:
cancelledis a misspelling ofcanceled(misspell)docs/api/devtime_dependencies.md (2)
17-29: Interface documentation looks goodThe interface definition is well-documented with clear descriptions of each method.
60-273: Excellent comprehensive examplesThe documentation provides two well-constructed examples that showcase:
- Basic integration with Redis using Testcontainers
- Advanced integration with dependency injection through Fiber's State
These examples will be very helpful for users to understand how to implement and use development-time dependencies.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
70-70: Hard tabs
Column: 1(MD010, no-hard-tabs)
71-71: Hard tabs
Column: 1(MD010, no-hard-tabs)
73-73: Hard tabs
Column: 1(MD010, no-hard-tabs)
74-74: Hard tabs
Column: 1(MD010, no-hard-tabs)
75-75: Hard tabs
Column: 1(MD010, no-hard-tabs)
79-79: Hard tabs
Column: 1(MD010, no-hard-tabs)
84-84: Hard tabs
Column: 1(MD010, no-hard-tabs)
85-85: Hard tabs
Column: 1(MD010, no-hard-tabs)
86-86: Hard tabs
Column: 1(MD010, no-hard-tabs)
87-87: Hard tabs
Column: 1(MD010, no-hard-tabs)
88-88: Hard tabs
Column: 1(MD010, no-hard-tabs)
90-90: Hard tabs
Column: 1(MD010, no-hard-tabs)
91-91: Hard tabs
Column: 1(MD010, no-hard-tabs)
97-97: Hard tabs
Column: 1(MD010, no-hard-tabs)
102-102: Hard tabs
Column: 1(MD010, no-hard-tabs)
103-103: Hard tabs
Column: 1(MD010, no-hard-tabs)
107-107: Hard tabs
Column: 1(MD010, no-hard-tabs)
109-109: Hard tabs
Column: 1(MD010, no-hard-tabs)
110-110: Hard tabs
Column: 1(MD010, no-hard-tabs)
111-111: Hard tabs
Column: 1(MD010, no-hard-tabs)
113-113: Hard tabs
Column: 1(MD010, no-hard-tabs)
115-115: Hard tabs
Column: 1(MD010, no-hard-tabs)
117-117: Hard tabs
Column: 1(MD010, no-hard-tabs)
118-118: Hard tabs
Column: 1(MD010, no-hard-tabs)
119-119: Hard tabs
Column: 1(MD010, no-hard-tabs)
120-120: Hard tabs
Column: 1(MD010, no-hard-tabs)
121-121: Hard tabs
Column: 1(MD010, no-hard-tabs)
122-122: Hard tabs
Column: 1(MD010, no-hard-tabs)
124-124: Hard tabs
Column: 1(MD010, no-hard-tabs)
125-125: Hard tabs
Column: 1(MD010, no-hard-tabs)
126-126: Hard tabs
Column: 1(MD010, no-hard-tabs)
127-127: Hard tabs
Column: 1(MD010, no-hard-tabs)
128-128: Hard tabs
Column: 1(MD010, no-hard-tabs)
129-129: Hard tabs
Column: 1(MD010, no-hard-tabs)
131-131: Hard tabs
Column: 1(MD010, no-hard-tabs)
132-132: Hard tabs
Column: 1(MD010, no-hard-tabs)
134-134: Hard tabs
Column: 1(MD010, no-hard-tabs)
135-135: Hard tabs
Column: 1(MD010, no-hard-tabs)
136-136: Hard tabs
Column: 1(MD010, no-hard-tabs)
137-137: Hard tabs
Column: 1(MD010, no-hard-tabs)
139-139: Hard tabs
Column: 1(MD010, no-hard-tabs)
152-152: Hard tabs
Column: 1(MD010, no-hard-tabs)
153-153: Hard tabs
Column: 1(MD010, no-hard-tabs)
154-154: Hard tabs
Column: 1(MD010, no-hard-tabs)
156-156: Hard tabs
Column: 1(MD010, no-hard-tabs)
157-157: Hard tabs
Column: 1(MD010, no-hard-tabs)
158-158: Hard tabs
Column: 1(MD010, no-hard-tabs)
162-162: Hard tabs
Column: 1(MD010, no-hard-tabs)
163-163: Hard tabs
Column: 1(MD010, no-hard-tabs)
164-164: Hard tabs
Column: 1(MD010, no-hard-tabs)
168-168: Hard tabs
Column: 1(MD010, no-hard-tabs)
173-173: Hard tabs
Column: 1(MD010, no-hard-tabs)
174-174: Hard tabs
Column: 1(MD010, no-hard-tabs)
175-175: Hard tabs
Column: 1(MD010, no-hard-tabs)
176-176: Hard tabs
Column: 1(MD010, no-hard-tabs)
177-177: Hard tabs
Column: 1(MD010, no-hard-tabs)
179-179: Hard tabs
Column: 1(MD010, no-hard-tabs)
180-180: Hard tabs
Column: 1(MD010, no-hard-tabs)
186-186: Hard tabs
Column: 1(MD010, no-hard-tabs)
191-191: Hard tabs
Column: 1(MD010, no-hard-tabs)
192-192: Hard tabs
Column: 1(MD010, no-hard-tabs)
196-196: Hard tabs
Column: 1(MD010, no-hard-tabs)
198-198: Hard tabs
Column: 1(MD010, no-hard-tabs)
199-199: Hard tabs
Column: 1(MD010, no-hard-tabs)
200-200: Hard tabs
Column: 1(MD010, no-hard-tabs)
202-202: Hard tabs
Column: 1(MD010, no-hard-tabs)
204-204: Hard tabs
Column: 1(MD010, no-hard-tabs)
206-206: Hard tabs
Column: 1(MD010, no-hard-tabs)
207-207: Hard tabs
Column: 1(MD010, no-hard-tabs)
208-208: Hard tabs
Column: 1(MD010, no-hard-tabs)
209-209: Hard tabs
Column: 1(MD010, no-hard-tabs)
210-210: Hard tabs
Column: 1(MD010, no-hard-tabs)
211-211: Hard tabs
Column: 1(MD010, no-hard-tabs)
213-213: Hard tabs
Column: 1(MD010, no-hard-tabs)
214-214: Hard tabs
Column: 1(MD010, no-hard-tabs)
215-215: Hard tabs
Column: 1(MD010, no-hard-tabs)
216-216: Hard tabs
Column: 1(MD010, no-hard-tabs)
217-217: Hard tabs
Column: 1(MD010, no-hard-tabs)
218-218: Hard tabs
Column: 1(MD010, no-hard-tabs)
220-220: Hard tabs
Column: 1(MD010, no-hard-tabs)
221-221: Hard tabs
Column: 1(MD010, no-hard-tabs)
223-223: Hard tabs
Column: 1(MD010, no-hard-tabs)
224-224: Hard tabs
Column: 1(MD010, no-hard-tabs)
225-225: Hard tabs
Column: 1(MD010, no-hard-tabs)
226-226: Hard tabs
Column: 1(MD010, no-hard-tabs)
228-228: Hard tabs
Column: 1(MD010, no-hard-tabs)
229-229: Hard tabs
Column: 1(MD010, no-hard-tabs)
231-231: Hard tabs
Column: 1(MD010, no-hard-tabs)
232-232: Hard tabs
Column: 1(MD010, no-hard-tabs)
233-233: Hard tabs
Column: 1(MD010, no-hard-tabs)
234-234: Hard tabs
Column: 1(MD010, no-hard-tabs)
235-235: Hard tabs
Column: 1(MD010, no-hard-tabs)
237-237: Hard tabs
Column: 1(MD010, no-hard-tabs)
238-238: Hard tabs
Column: 1(MD010, no-hard-tabs)
239-239: Hard tabs
Column: 1(MD010, no-hard-tabs)
240-240: Hard tabs
Column: 1(MD010, no-hard-tabs)
241-241: Hard tabs
Column: 1(MD010, no-hard-tabs)
243-243: Hard tabs
Column: 1(MD010, no-hard-tabs)
244-244: Hard tabs
Column: 1(MD010, no-hard-tabs)
245-245: Hard tabs
Column: 1(MD010, no-hard-tabs)
246-246: Hard tabs
Column: 1(MD010, no-hard-tabs)
247-247: Hard tabs
Column: 1(MD010, no-hard-tabs)
248-248: Hard tabs
Column: 1(MD010, no-hard-tabs)
250-250: Hard tabs
Column: 1(MD010, no-hard-tabs)
251-251: Hard tabs
Column: 1(MD010, no-hard-tabs)
253-253: Hard tabs
Column: 1(MD010, no-hard-tabs)
254-254: Hard tabs
Column: 1(MD010, no-hard-tabs)
256-256: Hard tabs
Column: 1(MD010, no-hard-tabs)
257-257: Hard tabs
Column: 1(MD010, no-hard-tabs)
258-258: Hard tabs
Column: 1(MD010, no-hard-tabs)
259-259: Hard tabs
Column: 1(MD010, no-hard-tabs)
261-261: Hard tabs
Column: 1(MD010, no-hard-tabs)
262-262: Hard tabs
Column: 1(MD010, no-hard-tabs)
263-263: Hard tabs
Column: 1(MD010, no-hard-tabs)
264-264: Hard tabs
Column: 1(MD010, no-hard-tabs)
265-265: Hard tabs
Column: 1(MD010, no-hard-tabs)
266-266: Hard tabs
Column: 1(MD010, no-hard-tabs)
267-267: Hard tabs
Column: 1(MD010, no-hard-tabs)
269-269: Hard tabs
Column: 1(MD010, no-hard-tabs)
270-270: Hard tabs
Column: 1(MD010, no-hard-tabs)
272-272: Hard tabs
Column: 1(MD010, no-hard-tabs)
🪛 GitHub Check: markdownlint
[failure] 86-86: Hard tabs
docs/api/devtime_dependencies.md:86:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 85-85: Hard tabs
docs/api/devtime_dependencies.md:85:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 84-84: Hard tabs
docs/api/devtime_dependencies.md:84:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 79-79: Hard tabs
docs/api/devtime_dependencies.md:79:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 75-75: Hard tabs
docs/api/devtime_dependencies.md:75:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 74-74: Hard tabs
docs/api/devtime_dependencies.md:74:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 73-73: Hard tabs
docs/api/devtime_dependencies.md:73:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 71-71: Hard tabs
docs/api/devtime_dependencies.md:71:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 70-70: Hard tabs
docs/api/devtime_dependencies.md:70:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces a new feature to enable the management of development-time dependencies for Fiber applications. Key changes include the addition of the DevTimeDependency interface and its lifecycle management (start and terminate), updates to application startup and shutdown to support dependencies, and comprehensive documentation with examples.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| listen.go | Adds log output for reporting the status of development dependencies |
| docs/whats_new.md | Updates the “What's New” documentation to include the new feature |
| docs/api/devtime_dependencies.md | Provides API documentation for the DevTimeDependency interface |
| docs/api/constants.md | Adjusts sidebar position to include new documentation entries |
| devtime_dependencies_test.go | Implements unit tests to verify dependency start and shutdown flows |
| devtime_dependencies.go | Introduces the core implementation and management functions |
| app.go | Updates application initialization to configure and manage the new dependencies |
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (10)
devtime_dependencies_test.go (3)
21-24:⚠️ Potential issueHandle context parameter in the Start method.
The mock currently ignores the context parameter in the Start method, which doesn't properly test context cancellation behavior.
func (m *mockDependency) Start(ctx context.Context) error { + select { + case <-ctx.Done(): + return ctx.Err() + default: + } m.started = true return m.startError }
30-33:⚠️ Potential issueHandle context parameter in the Terminate method.
Similarly, the mock ignores the context parameter in the Terminate method, which doesn't properly test context cancellation behavior.
func (m *mockDependency) Terminate(ctx context.Context) error { + select { + case <-ctx.Done(): + return ctx.Err() + default: + } m.terminated = true return m.terminateError }
154-175:⚠️ Potential issueSimilar issue with testing context cancellation for termination.
The Terminate test has the same issue as the Start test - it doesn't ensure the mock implementation actually respects context cancellation.
func TestDevTimeDependenciesTerminateWithContextCancellation(t *testing.T) { // Create a dependency that takes some time to terminate - slowDep := &mockDependency{name: "slow-dep"} + slowDep := &mockDependency{ + name: "slow-dep", + // Add artificial delay to ensure context actually times out + terminateFunc: func(ctx context.Context) error { + select { + case <-ctx.Done(): + return ctx.Err() + case <-time.After(100 * time.Millisecond): + return nil + } + }, + } app := &App{ configured: Config{ DevTimeDependencies: []DevTimeDependency{slowDep}, }, } // Start dependencies with cancelled context err := app.startDevTimeDependencies(context.Background()) require.NoError(t, err) // Create a new context for shutdown ctx, cancel := context.WithTimeout(context.Background(), 1*time.Nanosecond) defer cancel() // Shutdown dependencies with cancelled context err = app.shutdownDevTimeDependencies(ctx) require.ErrorIs(t, err, context.DeadlineExceeded) }docs/api/devtime_dependencies.md (7)
8-9: Fix consecutive blank lines.Remove the extra blank line to fix the markdown linting error.
Development-time services provide external dependencies needed to run the application while developing it. They are only supposed to be used while developing and are disabled when the application is deployed. - ## DevTimeDependency Interface
81-91:⚠️ Potential issueFix incorrect interface reference.
The comment references "fiber.RuntimeDependency" when it should be "fiber.DevTimeDependency".
-// Start initializes and starts the dependency. It implements the fiber.RuntimeDependency interface. +// Start initializes and starts the dependency. It implements the fiber.DevTimeDependency interface. func (s *redisStore) Start(ctx context.Context) error { // start the dependency c, err := tcredis.Run(ctx, "redis:latest") if err != nil { return err } s.ctr = c return nil }
93-97:⚠️ Potential issueFix incorrect interface reference in the String method comment.
The String method comment also references "fiber.RuntimeDependency" instead of "fiber.DevTimeDependency".
// String returns a human-readable representation of the dependency's state. -// It implements the fiber.RuntimeDependency interface. +// It implements the fiber.DevTimeDependency interface. func (s *redisStore) String() string { return "redis-store" }
99-103:⚠️ Potential issueFix incorrect interface reference in the Terminate method comment.
The Terminate method comment also references "fiber.RuntimeDependency" instead of "fiber.DevTimeDependency".
-// Terminate stops and removes the dependency. It implements the fiber.RuntimeDependency interface. +// Terminate stops and removes the dependency. It implements the fiber.DevTimeDependency interface. func (s *redisStore) Terminate(ctx context.Context) error { // stop the dependency return s.ctr.Terminate(ctx) }
170-180:⚠️ Potential issueFix incorrect interface reference in the second example.
The Start method comment in the second example also references "fiber.RuntimeDependency" incorrectly.
-// Start initializes and starts the dependency. It implements the fiber.RuntimeDependency interface. +// Start initializes and starts the dependency. It implements the fiber.DevTimeDependency interface. func (s *redisStore) Start(ctx context.Context) error { // start the dependency c, err := tcredis.Run(ctx, "redis:latest") if err != nil { return err } s.ctr = c return nil }
182-186:⚠️ Potential issueFix incorrect interface reference in the String method comment.
The String method comment in the second example also needs to be corrected.
// String returns a human-readable representation of the dependency's state. -// It implements the fiber.RuntimeDependency interface. +// It implements the fiber.DevTimeDependency interface. func (s *redisStore) String() string { return "redis-store" }
188-192:⚠️ Potential issueFix incorrect interface reference in the Terminate method comment.
The Terminate method comment in the second example also needs to be corrected.
-// Terminate stops and removes the dependency. It implements the fiber.RuntimeDependency interface. +// Terminate stops and removes the dependency. It implements the fiber.DevTimeDependency interface. func (s *redisStore) Terminate(ctx context.Context) error { // stop the dependency return s.ctr.Terminate(ctx) }
🧹 Nitpick comments (5)
devtime_dependencies_benchmark_test.go (2)
16-21: Consider handling context cancellation in the benchmark dependency.While this is a benchmark implementation, the mock should ideally respect context cancellation to accurately benchmark real-world behavior.
func (m *benchmarkDependency) Start(ctx context.Context) error { + select { + case <-ctx.Done(): + return ctx.Err() + default: + } if m.startDelay > 0 { time.Sleep(m.startDelay) } return nil }
27-32: Consider handling context cancellation in the Terminate method.Similar to the Start method, the Terminate implementation should respect context cancellation.
func (m *benchmarkDependency) Terminate(ctx context.Context) error { + select { + case <-ctx.Done(): + return ctx.Err() + default: + } if m.terminateDelay > 0 { time.Sleep(m.terminateDelay) } return nil }devtime_dependencies_test.go (1)
13-19: Optimize struct field alignment and size.The
mockDependencystruct has a suboptimal field alignment that could be improved to reduce its memory footprint.// mockDependency implements DevTimeDependency interface for testing type mockDependency struct { name string + started bool + terminated bool startError error terminateError error - started bool - terminated bool }🧰 Tools
🪛 GitHub Check: lint
[failure] 13-13:
fieldalignment: struct with 48 pointer bytes could be 40 (govet)devtime_dependencies.go (1)
58-62: Consider different error handling on context cancellation.When a context is canceled during shutdown, the code currently appends the error to the error list and continues with other dependencies. This differs from the start method's approach where context cancellation causes an immediate return. It might be beneficial to have consistent behavior.
Consider this alternative approach for handling context cancellation during shutdown:
func (app *App) shutdownDevTimeDependencies(ctx context.Context) error { if app.hasDevTimeDependencies() { var errs []error for _, dep := range app.configured.DevTimeDependencies { if err := ctx.Err(); err != nil { - // Context is canceled, do a best effort to terminate the dependencies. - errs = append(errs, fmt.Errorf("context canceled: %w", err)) - continue + // Context is canceled, return an error immediately, consistent with startDevTimeDependencies + return fmt.Errorf("context canceled while terminating dependencies: %w", err) } err := dep.Terminate(ctx) if err != nil { if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { return fmt.Errorf("dependency %s terminate: %w", dep.String(), err) } errs = append(errs, fmt.Errorf("terminate dependency %s: %w", dep.String(), err)) } } return errors.Join(errs...) } return nil }Alternatively, explain in a comment why the behavior is different between start and shutdown:
if err := ctx.Err(); err != nil { - // Context is canceled, do a best effort to terminate the dependencies. + // Context is canceled, but we continue with a best effort to terminate all dependencies + // unlike startDevTimeDependencies which returns immediately, because it's more important + // to attempt to clean up all resources even if the context is canceled. errs = append(errs, fmt.Errorf("context canceled: %w", err)) continue }docs/api/devtime_dependencies.md (1)
227-229: Consider using dependency injection middleware instead of state.Using the app's state for dependency injection works, but consider suggesting a middleware-based approach that provides a more structured way to inject dependencies into the request handlers.
// Create a middleware to inject Redis client func WithRedis(client *redis.Client) fiber.Handler { return func(c fiber.Ctx) error { c.Locals("redis", client) return c.Next() } } // In your routes setup: app.Use(WithRedis(rdb)) // Then in your handlers: app.Post("/user/create", func(c fiber.Ctx) error { // Get Redis client from context rdb := c.Locals("redis").(*redis.Client) // Use the client... })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
devtime_dependencies.go(1 hunks)devtime_dependencies_benchmark_test.go(1 hunks)devtime_dependencies_test.go(1 hunks)docs/api/devtime_dependencies.md(1 hunks)docs/whats_new.md(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/whats_new.md
🧰 Additional context used
🧬 Code Graph Analysis (1)
devtime_dependencies_benchmark_test.go (1)
devtime_dependencies.go (1)
DevTimeDependency(10-20)
🪛 GitHub Check: lint
devtime_dependencies_benchmark_test.go
[failure] 162-162:
Error return value of app.shutdownDevTimeDependencies is not checked (errcheck)
[failure] 161-161:
Error return value of app.startDevTimeDependencies is not checked (errcheck)
[failure] 150-150:
test helper function should start from b.Helper() (thelper)
[failure] 129-129:
Error return value of app.startDevTimeDependencies is not checked (errcheck)
[failure] 119-119:
test helper function should start from b.Helper() (thelper)
[failure] 87-87:
Error return value of app.shutdownDevTimeDependencies is not checked (errcheck)
[failure] 77-77:
test helper function should start from b.Helper() (thelper)
[failure] 45-45:
Error return value of app.startDevTimeDependencies is not checked (errcheck)
[failure] 35-35:
test helper function should start from b.Helper() (thelper)
devtime_dependencies_test.go
[failure] 13-13:
fieldalignment: struct with 48 pointer bytes could be 40 (govet)
🪛 GitHub Actions: golangci-lint
devtime_dependencies_benchmark_test.go
[error] 35-35: golangci-lint: test helper function should start from b.Helper() (thelper)
🪛 GitHub Check: codecov/patch
devtime_dependencies.go
[warning] 42-43: devtime_dependencies.go#L42-L43
Added lines #L42 - L43 were not covered by tests
[warning] 67-68: devtime_dependencies.go#L67-L68
Added lines #L67 - L68 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: unit (1.24.x, macos-13)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: unit (1.23.x, macos-13)
- GitHub Check: repeated
- GitHub Check: Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
devtime_dependencies_benchmark_test.go (4)
34-49: Check error returns from startDevTimeDependencies.The error returned by
app.startDevTimeDependenciesis being discarded, which is flagged by the linter. Error checking is important even in benchmarks to ensure the benchmark is measuring what it's supposed to.Apply this fix as mentioned in previous reviews:
for i := 0; i < b.N; i++ { ctx := context.Background() - _ = app.startDevTimeDependencies(ctx) + if err := app.startDevTimeDependencies(ctx); err != nil { + b.Fatal("Failed to start dependencies:", err) + } }🧰 Tools
🪛 GitHub Check: lint
[failure] 47-47:
Error return value ofapp.startDevTimeDependenciesis not checked (errcheck)
78-93: Check error returns from shutdownDevTimeDependencies.Similar to the start benchmark, the error returned by
app.shutdownDevTimeDependenciesis being discarded.Apply this fix as mentioned in previous reviews:
for i := 0; i < b.N; i++ { ctx := context.Background() - _ = app.shutdownDevTimeDependencies(ctx) + if err := app.shutdownDevTimeDependencies(ctx); err != nil { + b.Fatal("Failed to shutdown dependencies:", err) + } }🧰 Tools
🪛 GitHub Check: lint
[failure] 91-91:
Error return value ofapp.shutdownDevTimeDependenciesis not checked (errcheck)
122-138: Check and validate errors from context cancellation benchmark.The error from
app.startDevTimeDependenciesis discarded, but with an immediate timeout (1 nanosecond), we expect an error to occur due to context cancellation. The benchmark should validate this behavior.Apply this fix as suggested in previous reviews:
for i := 0; i < b.N; i++ { ctx, cancel := context.WithTimeout(context.Background(), timeout) - _ = app.startDevTimeDependencies(ctx) + err := app.startDevTimeDependencies(ctx) + // We expect an error here due to the short timeout + if err == nil && timeout < time.Microsecond { + b.Fatal("Expected error due to context cancellation but got none") + } cancel() }🧰 Tools
🪛 GitHub Check: lint
[failure] 135-135:
Error return value ofapp.startDevTimeDependenciesis not checked (errcheck)
155-172: Check errors in memory benchmark.The memory benchmark ignores errors from both
startDevTimeDependenciesandshutdownDevTimeDependencies, which should be checked to ensure the benchmark is valid.Apply this fix as suggested in previous reviews:
for i := 0; i < b.N; i++ { ctx := context.Background() - _ = app.startDevTimeDependencies(ctx) - _ = app.shutdownDevTimeDependencies(ctx) + if err := app.startDevTimeDependencies(ctx); err != nil { + b.Fatal("Failed to start dependencies:", err) + } + if err := app.shutdownDevTimeDependencies(ctx); err != nil { + b.Fatal("Failed to shutdown dependencies:", err) + } }🧰 Tools
🪛 GitHub Check: lint
[failure] 170-170:
Error return value ofapp.shutdownDevTimeDependenciesis not checked (errcheck)
[failure] 169-169:
Error return value ofapp.startDevTimeDependenciesis not checked (errcheck)
🧹 Nitpick comments (2)
devtime_dependencies_benchmark_test.go (2)
10-14: Optimize struct field layout for better memory efficiency.The
benchmarkDependencystruct has a suboptimal memory layout. Go's struct field alignment affects memory usage and performance, especially in performance-sensitive code like benchmarks.Reordering the fields can reduce the struct size from 24 to 8 bytes:
type benchmarkDependency struct { - startDelay time.Duration - terminateDelay time.Duration name string + startDelay time.Duration + terminateDelay time.Duration }🧰 Tools
🪛 GitHub Check: lint
[failure] 10-10:
fieldalignment: struct with 24 pointer bytes could be 8 (govet)🪛 GitHub Actions: golangci-lint
[error] 10-10: golangci-lint: fieldalignment: struct with 24 pointer bytes could be 8 (govet)
140-152: Consider adding more diverse timeout values for context cancellation tests.Currently, the benchmark only tests with an immediate cancellation (1 nanosecond). Consider adding tests with varying timeouts to better understand the performance characteristics under different cancellation scenarios.
Add a test case with a longer timeout that still triggers cancellation:
b.Run("multiple-dependencies/delayed-cancellation", func(b *testing.B) { benchmarkFn(b, []DevTimeDependency{ &benchmarkDependency{name: "dep1", startDelay: 10 * time.Millisecond}, &benchmarkDependency{name: "dep2", startDelay: 20 * time.Millisecond}, &benchmarkDependency{name: "dep3", startDelay: 30 * time.Millisecond}, }, 15*time.Millisecond) // Cancels after first dependency starts but before others })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
devtime_dependencies_benchmark_test.go(1 hunks)devtime_dependencies_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- devtime_dependencies_test.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
devtime_dependencies_benchmark_test.go (1)
devtime_dependencies.go (1)
DevTimeDependency(10-20)
🪛 GitHub Check: lint
devtime_dependencies_benchmark_test.go
[failure] 170-170:
Error return value of app.shutdownDevTimeDependencies is not checked (errcheck)
[failure] 169-169:
Error return value of app.startDevTimeDependencies is not checked (errcheck)
[failure] 135-135:
Error return value of app.startDevTimeDependencies is not checked (errcheck)
[failure] 91-91:
Error return value of app.shutdownDevTimeDependencies is not checked (errcheck)
[failure] 47-47:
Error return value of app.startDevTimeDependencies is not checked (errcheck)
[failure] 10-10:
fieldalignment: struct with 24 pointer bytes could be 8 (govet)
🪛 GitHub Actions: golangci-lint
devtime_dependencies_benchmark_test.go
[error] 10-10: golangci-lint: fieldalignment: struct with 24 pointer bytes could be 8 (govet)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: unit (1.24.x, macos-latest)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: unit (1.24.x, macos-13)
- GitHub Check: unit (1.23.x, macos-latest)
- GitHub Check: unit (1.23.x, macos-13)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: Compare
- GitHub Check: repeated
- GitHub Check: Analyse
🔇 Additional comments (3)
devtime_dependencies_benchmark_test.go (3)
16-21: LGTM: Simple and effective context-ignoring implementation.The Start method correctly simulates delays while ignoring the context parameter, which is appropriate for these simple benchmarks.
23-25: LGTM: Straightforward String implementation.This method correctly returns the name of the benchmark dependency, fulfilling the interface requirement.
27-32: LGTM: Terminate implementation matches Start pattern.The Terminate method appropriately mirrors the Start method's implementation with delay simulation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
devtime_dependencies_prod.go (4)
1-2: Remove redundant build tagSince Go 1.17, the older style build tag format
// +build !devis redundant when you're already using the modern//go:build !devdirective.//go:build !dev - // +build !dev🧰 Tools
🪛 GitHub Check: lint
[failure] 2-2:
redundant-build-tag: The build tag "// +build" is redundant since Go 1.17 and can be removed (revive)
19-21: Rename unused receiver variableThe receiver variable
appis not used in this method. Consider renaming it to_to indicate it's intentionally unused.- func (app *App) hasDevTimeDependencies() bool { + func (_ *App) hasDevTimeDependencies() bool { return false }🧰 Tools
🪛 GitHub Check: lint
[failure] 19-19:
unused-receiver: method receiver 'app' is not referenced in method's body, consider removing or renaming it as _ (revive)
24-26: Receiver variable is already properly named for unused parameterThis function correctly uses
_for the unused context parameter, but the receiver variableappis still not used. Consider renaming it as well.- func (app *App) startDevTimeDependencies(_ context.Context) error { + func (_ *App) startDevTimeDependencies(_ context.Context) error { return nil }🧰 Tools
🪛 GitHub Check: lint
[failure] 24-24:
unused-receiver: method receiver 'app' is not referenced in method's body, consider removing or renaming it as _ (revive)
29-31: Rename unused receiver variableThe receiver variable
appis not used in this method. Consider renaming it to_to indicate it's intentionally unused.- func (app *App) shutdownDevTimeDependencies(_ context.Context) error { + func (_ *App) shutdownDevTimeDependencies(_ context.Context) error { return nil }🧰 Tools
🪛 GitHub Check: lint
[failure] 29-29:
unused-receiver: method receiver 'app' is not referenced in method's body, consider removing or renaming it as _ (revive)devtime_dependencies_dev.go (4)
26-29: Fix comment style for methodGo method comments should begin with the method name. This helps generate proper documentation with
godoc.- // hasDevTimeDependencies Checks if there are any dependency for the current application. + // hasDevTimeDependencies checks if there are any dependencies for the current application.Also, there's a minor grammatical error in the comment (should be "dependencies" plural rather than "dependency").
31-54: Logic for starting dependencies looks goodYou've implemented proper error handling and context cancellation for starting dependencies. The approach of immediately returning on context cancellation ensures that users can quickly respond to cancellation events.
One small grammatical improvement:
- // startDevTimeDependencies Handles the start process of dependencies for the current application. + // startDevTimeDependencies handles the start process of dependencies for the current application.
56-79: Different error handling strategies between start and shutdownI noticed that the error handling strategy differs between
startDevTimeDependenciesandshutdownDevTimeDependencies:
- In
startDevTimeDependencies, if context is canceled, you return immediately- In
shutdownDevTimeDependencies, if context is canceled, you continue with best-effort terminationThis difference is likely intentional to ensure all dependencies attempt termination even if the context is canceled, but it might be worth adding a comment explaining this design decision for future maintainers.
- // shutdownDevTimeDependencies Handles the shutdown process of dependencies for the current application. - // Iterates over all dependencies and tries to terminate them, returning an error if any error occurs. + // shutdownDevTimeDependencies handles the shutdown process of dependencies for the current application. + // Iterates over all dependencies and tries to terminate them, returning an error if any error occurs. + // Unlike startDevTimeDependencies, this method continues with best-effort termination even if the context + // is canceled to ensure as many dependencies as possible are properly terminated.
81-91: Logging assumes all dependencies successfully startedThe logging implementation assumes all dependencies are in "OK" state (line 88). Consider modifying the logging to handle cases where dependencies might have failed to start, or make it clear this only logs started dependencies.
You could consider a more robust approach that shows the status of each dependency:
func (app *App) logDevTimeDependencies(out io.Writer, colors Colors) { if app.hasDevTimeDependencies() { fmt.Fprintf(out, "%sINFO%s Dev-time dependencies: \t%s%d%s\n", colors.Green, colors.Reset, colors.Blue, len(app.configured.DevTimeDependencies), colors.Reset) for _, dep := range app.configured.DevTimeDependencies { - fmt.Fprintf(out, "%sINFO%s 🥡 %s[ OK ] %s%s\n", colors.Green, colors.Reset, colors.Blue, dep.String(), colors.Reset) + // Assuming you track status of dependencies in the app or add a Status() method to the interface + status := "[ OK ]" + statusColor := colors.Blue + // if depFailed(dep) { // This would require tracking dependency start status + // status = "[FAIL]" + // statusColor = colors.Red + // } + fmt.Fprintf(out, "%sINFO%s 🥡 %s%s %s%s\n", colors.Green, colors.Reset, statusColor, status, dep.String(), colors.Reset) } } }Alternatively, fix the comment to clarify this only logs started dependencies:
- // logDevTimeDependencies logs information about development-time dependencies + // logDevTimeDependencies logs information about successfully started development-time dependencies
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
devtime_dependencies_dev.go(1 hunks)devtime_dependencies_prod.go(1 hunks)listen.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- listen.go
🧰 Additional context used
🪛 GitHub Check: lint
devtime_dependencies_prod.go
[failure] 29-29:
unused-receiver: method receiver 'app' is not referenced in method's body, consider removing or renaming it as _ (revive)
[failure] 24-24:
unused-receiver: method receiver 'app' is not referenced in method's body, consider removing or renaming it as _ (revive)
[failure] 19-19:
unused-receiver: method receiver 'app' is not referenced in method's body, consider removing or renaming it as _ (revive)
[failure] 2-2:
redundant-build-tag: The build tag "// +build" is redundant since Go 1.17 and can be removed (revive)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Compare
- GitHub Check: Analyse
- GitHub Check: repeated
🔇 Additional comments (3)
devtime_dependencies_prod.go (2)
11-16: Interface definition looks goodThe
DevTimeDependencyinterface is well-defined with the three essential methods for managing development-time dependencies:Start,String, andTerminate.
34-34: LGTMThe no-op implementation for
logDevTimeDependencieslooks good. You've correctly defined the function with unused parameters properly marked with underscores.devtime_dependencies_dev.go (1)
13-24: Interface definition with documentation looks goodThe
DevTimeDependencyinterface is well-documented with clear descriptions for each method. This will help developers implement their own development-time dependencies.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
state_test.go (1)
581-593: LGTM: Variable renamed to avoid shadowing built-in.The variable has been properly renamed from
lentoservicesLento avoid shadowing the built-inlen()function, as requested in the previous review.
🧹 Nitpick comments (1)
state_test.go (1)
528-726: Consider adding benchmark tests for service operations.The test suite thoroughly validates functionality but doesn't include benchmarks for the new service operations. Consider adding benchmarks similar to the existing ones for regular state operations to measure performance impact.
func BenchmarkState_GetService(b *testing.B) { b.ReportAllocs() st := newState() srv := &mockService{name: "benchService"} st.setService(srv) b.ResetTimer() for i := 0; i < b.N; i++ { _, _ = GetService[*mockService](st, srv.String()) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
state_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: unit (1.23.x, macos-13)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (4)
state_test.go (4)
528-669: Well-structured test coverage for the new Service functionality.The comprehensive test suite for service operations is well organized with clear test cases covering all aspects of service management including setting, getting, enumeration, and deletion. The use of parallel subtests increases efficiency and helps isolate test cases.
532-533: Good test approach using subtle naming differences.Using a service with a subtle name difference (adding a space) is a smart way to verify that the service key generation handles similar service names correctly.
671-698: Good type-safe service retrieval test.The
TestState_GetServicefunction properly tests the generic retrieval functionality with appropriate type safety using Go's generics. The test covers both successful retrieval and failure cases.
700-726: Well-designed panic test for MustGetService.The test correctly verifies that
MustGetServicereturns the expected service when available and panics when the service doesn't exist, providing complete coverage of the intended behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
services_test.go (1)
280-304: LGTM! Test for shutdown with canceled context properly implementedThis test correctly verifies the behavior of shutting down services with an already canceled context. It aligns with the previous review comment that suggested adding this test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
services_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Analyse
- GitHub Check: Compare
🔇 Additional comments (2)
services_test.go (2)
20-109: LGTM! Well-implemented mock service with proper context handlingThe mock service implementation is thorough and properly handles various scenarios including context cancellation, delays, error conditions, and state tracking. This provides a solid foundation for testing the service lifecycle management features.
619-659: LGTM! Well-designed memory benchmarkThe memory allocation benchmark is properly implemented with
ReportAllocs()and tests a variety of scenarios from no services to multiple services. This will help ensure the feature maintains good memory performance characteristics.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
state.go (2)
407-411: Add tests for GetService function.This generic function for type-safe service retrieval is well-implemented, but static analysis indicates it's not covered by tests. Please add tests to verify this core functionality.
#!/bin/bash # Search for any tests of GetService function rg -n "GetService\[" -g "*_test.go" # Also search for any test cases that might indirectly test this rg -n "GetService" -g "*_test.go"
413-422: Make error message consistent and add tests for MustGetService.The panic message is inconsistent with similar functions. Consider using "state: dependency not found!" to match
MustGetor make both use a standardized message. Also, please add tests for this function.func MustGetService[T Service](s *State, key string) T { srv, ok := GetService[T](s, key) if !ok { - panic("state: service not found!") + panic("state: dependency not found!") } return srv }
🧹 Nitpick comments (1)
state.go (1)
362-380: Add error logging for type assertion failures.The method correctly filters keys by the service prefix, but silently continues when a type assertion fails. Consider adding error logging to help diagnose state corruption.
func (s *State) serviceKeys() []string { keys := make([]string, 0) s.dependencies.Range(func(key, _ any) bool { keyStr, ok := key.(string) if !ok { - return false + log.Printf("state: non-string key found in dependencies: %v", key) + return true // Continue iterating } if !strings.HasPrefix(keyStr, s.servicePrefix) { return true // Continue iterating if key doesn't have service prefix } keys = append(keys, keyStr) return true }) return keys }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 368-369: state.go#L368-L369
Added lines #L368 - L369 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
state.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
state.go (2)
app.go (1)
New(522-641)services.go (1)
Service(12-25)
🪛 GitHub Check: codecov/patch
state.go
[warning] 368-369: state.go#L368-L369
Added lines #L368 - L369 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Compare
- GitHub Check: unit (1.24.x, macos-latest)
- GitHub Check: repeated
- GitHub Check: unit (1.23.x, macos-13)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: unit (1.23.x, windows-latest)
🔇 Additional comments (6)
state.go (6)
4-19: Implementation of a secure, collision-resistant service namespace looks good.The addition of a random, hashed prefix for service keys is a well-designed approach to namespace isolation. Using UUID combined with SHA-256 hashing creates a secure, unique identifier that prevents collisions with other state entries.
The implementation correctly uses
hex.EncodeToString()to ensure the hash is represented as printable characters, addressing the issues mentioned in previous reviews.
24-33: Good separation of concerns between general dependencies and services.Adding a dedicated
servicePrefixfield to theStatestruct provides a clean separation between general dependencies and services, making the code more maintainable and less prone to key collisions.
341-349: Well-implemented service key generation.The
serviceKeyfunction properly generates a unique, deterministic key for each service by hashing the service name and prefixing it with the state's service prefix. The use ofhex.EncodeToStringensures all keys are printable, addressing the previous review comment about non-printable bytes.
351-355: LGTM: Service storage implementation.The
setServicefunction correctly uses the service'sString()method to generate a consistent key, ensuring services can be reliably retrieved.
357-360: LGTM: Service deletion implementation.The
deleteServicefunction consistently uses the same key generation approach assetService, ensuring proper service cleanup.
394-405: Good optimization in ServicesLen().The
ServicesLen()method efficiently counts services without constructing an intermediate slice, which is more memory-efficient than usingserviceKeys()and then getting the length.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
state.go (1)
383-385: UsingMustGetStatehere can panic – prefer safe retrieval
This was pointed out in an earlier review: if a stored value is not of typeService, the whole call panics. ConsiderGetStateplus a type-check to fail gracefully.
🧹 Nitpick comments (5)
state.go (5)
11-17: Misleading “hash” wording – the prefix is only hex-encoded
servicesStatePrefixHashis built withhex.EncodeToString([]byte(...)), which just converts the raw ASCII bytes to hex; no hashing happens. The constant/docstring repeatedly says “hash” which may confuse future readers.-// servicesStatePrefix is hashed and hex-encoded to avoid collisions -const servicesStatePrefix = "gofiber-services-" +// Prefix used to namespace service keys inside State. +// It is later hex-encoded so that the final key is printable. +const servicesStatePrefix = "gofiber-services-" ... -func init() { - servicesStatePrefixHash = hex.EncodeToString([]byte(servicesStatePrefix + uuid.New().String())) -} +func init() { + // Use a random UUID so every process gets a unique, printable namespace. + servicesStatePrefixHash = hex.EncodeToString([]byte(servicesStatePrefix + uuid.New().String())) +}Alternatively, if you really want a hash, compute
sha256.Sum256then hex-encode the result.
28-32: Comment says “hashed” but implementation doesn’tThe comment above
newState()claims we “Initialize the services state prefix using a hashed random string”, yet the code only hex-encodes. Either update the comment or actually hash (e.g.sha256.Sum256).
339-345:serviceKeydoes not hash; update wording or hash properlyThe function comment and inline note mention “hash the service string to avoid collisions”, but the code simply performs
hex.EncodeToString([]byte(key)). This still leaves open collisions if two services differ only in case (ASCII hex is lowercase) or happen to produce the same hex once encoded (unlikely but possible).Two options:
- Keep the simple encoding – just fix the comment:
- // hash the service string to avoid collisions - return s.servicePrefix + hex.EncodeToString([]byte(key)) + // Hex-encode to keep keys printable and avoid separators collisions + return s.servicePrefix + hex.EncodeToString([]byte(key))
- Actually hash:
+ hash := sha256.Sum256([]byte(key)) + return s.servicePrefix + hex.EncodeToString(hash[:])Either way, keep docs and code in sync.
353-356: Comment/identifier mismatch fordeleteServiceGo doc comments must start with the exact function name. Current wording starts with
Delete, but the function isdeleteService, which will triggergolintwarnings.-// Delete removes a key-value pair from the State. +// deleteService removes a Service from the State.
360-368:serviceKeys()stops iteration prematurely on non-string keysReturning
falsefromRangehalts the traversal. If any rogue non-string key sneaks in, the remaining service keys won’t be discovered.- keyStr, ok := key.(string) - if !ok { - return false - } + keyStr, ok := key.(string) + if !ok { + return true // skip keys that are not strings, continue scanning + }This makes the method resilient without impacting happy paths.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 364-365: state.go#L364-L365
Added lines #L364 - L365 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
state.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
state.go
[warning] 364-365: state.go#L364-L365
Added lines #L364 - L365 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: unit (1.24.x, macos-latest)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: Analyse
- GitHub Check: repeated
- GitHub Check: Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
services.go (2)
112-128: 🛠️ Refactor suggestionTerminate services in reverse order to respect dependency chains (same concern raised previously)
Many systems assume that teardown happens in the opposite order of startup (e.g. DB → cache → app).
Iterating in insertion order may break dependent services. A previous comment suggested reversing the slice; the current implementation still traverses in the original order.- for _, srv := range app.state.Services() { + for i := len(app.state.Services()) - 1; i >= 0; i-- { + srv := app.state.Services()[i]
79-100:⚠️ Potential issueLeak risk: no rollback of already-started services when a later start fails
If service N fails to start, services 0…N-1 remain running and stay in
app.state, potentially leaking resources and leaving the app in a half-initialised state.Add a best-effort rollback that terminates any services started earlier before returning the error:
- var errs []error - for _, srv := range app.configured.Services { + var ( + errs []error + started []Service // successfully started, to allow rollback + ) + for _, srv := range app.configured.Services { … - err := srv.Start(ctx) - if err == nil { - // mark the service as started - app.state.setService(srv) - continue - } + err := srv.Start(ctx) + if err == nil { + started = append(started, srv) + app.state.setService(srv) + continue + } + + // Roll back anything that has already started. + for i := len(started) - 1; i >= 0; i-- { + _ = started[i].Terminate(ctx) // best effort + app.state.deleteService(started[i]) + }
🧹 Nitpick comments (3)
services.go (1)
29-37: Doc-comment formatting does not follow Go conventionsThe first word in a top-level comment should be the identifier it describes and start with a capital letter (“hasConfiguredServices ...”), or—since the function is unexported—omit the comment entirely.
Keeping comments consistent with Go doc tooling avoids linter noise.-// hasConfiguredServices Checks if there are any services for the current application. +// hasConfiguredServices reports whether the application has at least one service +// configured via Config.Services.services_test.go (2)
180-189: Global logger redirection isn’t restored, causing test interference
log.SetOutput(&buf)changes package-wide state for the whole test process.
Restore the previous writer to avoid races with parallel tests and benchmarks:- var buf stringsLogger - log.SetOutput(&buf) + var buf stringsLogger + prev := log.Writer() + log.SetOutput(&buf) + t.Cleanup(func() { log.SetOutput(prev) }) // restore after testAlso applies to: 211-217
501-510: Benchmarks callstartServicestwice, skewing results
New(Config{Services: …})already starts services internally, so the extra
app.startServices(ctx):
- Measures duplicate work that won’t happen in real code.
- Risks double-start errors for real implementations.
Removing the explicit call gives more realistic numbers.
- ctx := context.Background() - if err := app.startServices(ctx); err != nil { - b.Fatal("Expected no error but got", err) - }(Apply similarly in other benchmark helpers.)
Also applies to: 653-656, 724-730
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app.go(4 hunks)services.go(1 hunks)services_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app.go
🧰 Additional context used
🧠 Learnings (1)
services_test.go (2)
Learnt from: mdelapenya
PR: gofiber/fiber#3434
File: services_test.go:450-464
Timestamp: 2025-05-15T12:56:45.397Z
Learning: The `New` function in the Fiber framework automatically calls `startServices` at initialization time when services are configured, making explicit calls to `startServices` unnecessary in code that creates an App instance with `New()`.
Learnt from: mdelapenya
PR: gofiber/fiber#3434
File: services_test.go:450-464
Timestamp: 2025-05-15T12:56:45.397Z
Learning: In the Fiber framework, the `New` function automatically calls `startServices` when services are configured, so there's no need to explicitly call `startServices` after creating a new app with `New(Config{Services: ...})`.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Compare
* main: Update AGENTS.md Create AGENTS.md 🐛 bug: fix redirection flash messages violate cookie structure (gofiber#3457) build(deps): bump codecov/codecov-action from 5.4.2 to 5.4.3 🚀 Improve routing treeBuild flow (gofiber#3456) build(deps): bump github.com/tinylib/msgp from 1.2.5 to 1.3.0 (gofiber#3447) build(deps): bump DavidAnson/markdownlint-cli2-action from 19 to 20 🔥 feat: Add All method to Bind (gofiber#3373) 📝 docs: Document usage of Custom Tags in Logger middleware (gofiber#3446)
* main: 📚 Doc: fix AGENTS markdown lint (gofiber#3460)
This reverts commit bb67cf6.
Description
This pull request introduces a new feature called Services to Fiber v3, which allows developers to quickly start and manage development/test-time dependencies for their applications. This feature, inspired in the Spring Boot Java framework, eliminates the need to manually provision services like databases, caches, or message brokers during development, streamlining the development workflow and ensuring consistent development or testing environments across team members.
The implementation provides a clean interface for managing dependencies through the
Serviceinterface, which handles the lifecycle of development dependencies including starting and terminating them, and checking its state. This feature is particularly valuable for teams working on microservices or applications with multiple dependencies, as it simplifies the development setup process and reduces configuration overhead.The configured Services are stored in the Global State, so this PR is providing public functions to get a service from the Global State.
Changes introduced
New Features
Serviceinterface for managing development-time dependenciesCode Changes
services.gofile with core implementationDocumentation
Added comprehensive documentation for the new feature
Included usage examples and best practices (a redis container started through testcontainers, including global State usage)
Updated State docs with new Service functions, linking to Services docs
Benchmarks: Describe any performance benchmarks and improvements related to the changes.
Documentation Update: Detail the updates made to the documentation and links to the changed files.
Changelog/What's New: Include a summary of the additions for the upcoming release notes.
Migration Guide: If necessary, provide a guide or steps for users to migrate their existing code to accommodate these changes.
API Alignment with
ExpressSpring Boot: Explain how the changes align with theExpressSpring Boot API.API Longevity: Discuss the steps taken to ensure that the new or updated APIs are consistent and not prone to breaking changes.
Examples: Provide examples demonstrating the new features or changes in action.
Type of change
Please delete options that are not relevant.
Checklist
Before you submit your pull request, please make sure you meet these requirements:
Express.jsSpringBoot framework for new functionalities, making them similar in usage./docs/directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md