Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
📝 WalkthroughWalkthroughThis update introduces a comprehensive CLI framework for the Go-based Unkey project, including typed flag parsing, subcommand support, help output, and command execution logic. It implements a full deployment workflow for Docker-based applications, integrating Docker build/push, Git metadata, and a control plane API for version management. The CLI supports deployment, version management, and initialization commands, with interactive UI feedback and robust error handling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI_Main
participant CommandParser
participant DeployCmd
participant Docker
participant ControlPlane
participant UI
User->>CLI_Main: Run CLI (e.g., unkey deploy ...)
CLI_Main->>CommandParser: Parse args, match command
CommandParser->>DeployCmd: Execute DeployAction
DeployCmd->>UI: Print source info, start spinner
DeployCmd->>Docker: Build image (if needed)
Docker-->>DeployCmd: Build result
DeployCmd->>Docker: Push image (if not skipped)
Docker-->>DeployCmd: Push result
DeployCmd->>ControlPlane: Create version (API call)
ControlPlane-->>DeployCmd: Version ID
DeployCmd->>ControlPlane: Poll version status/steps
ControlPlane-->>DeployCmd: Status/step events
DeployCmd->>UI: Update spinner, print progress
DeployCmd->>UI: Print completion info
DeployCmd-->>User: Exit with success or error
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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. 🪧 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 (
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
go/apps/api/cancel_test.go(5 hunks)go/apps/api/config.go(2 hunks)go/apps/api/integration/harness.go(5 hunks)go/apps/api/run.go(3 hunks)go/cmd/api/main.go(3 hunks)go/pkg/listener/doc.go(1 hunks)go/pkg/listener/ephemeral.go(1 hunks)go/pkg/listener/listener.go(1 hunks)go/pkg/listener/port.go(1 hunks)go/pkg/port/doc.go(0 hunks)go/pkg/port/free.go(0 hunks)go/pkg/zen/README.md(6 hunks)go/pkg/zen/doc.go(1 hunks)go/pkg/zen/server.go(3 hunks)go/pkg/zen/server_tls_test.go(5 hunks)
💤 Files with no reviewable changes (2)
- go/pkg/port/doc.go
- go/pkg/port/free.go
🧰 Additional context used
🧠 Learnings (3)
go/apps/api/integration/harness.go (2)
Learnt from: Flo4604
PR: unkeyed/unkey#3606
File: go/pkg/prometheus/metrics/database.go:29-30
Timestamp: 2025-07-16T10:06:35.397Z
Learning: In Go packages, variables defined in one file within a package (like `latencyBuckets` and `constLabels` in go/pkg/prometheus/metrics/http.go) are accessible from other files in the same package without requiring imports. This is a common pattern for sharing configuration across multiple files within a package.
Learnt from: chronark
PR: unkeyed/unkey#3560
File: go/deploy/metald/internal/database/repository.go:0-0
Timestamp: 2025-07-15T14:59:30.212Z
Learning: go/deploy/metald cannot currently import helpers from go/pkg/db because it is not yet part of the main Go module; avoid suggesting such imports until the modules are unified.
go/pkg/zen/README.md (4)
Learnt from: Flo4604
PR: unkeyed/unkey#3606
File: go/pkg/prometheus/metrics/database.go:29-30
Timestamp: 2025-07-16T10:06:35.397Z
Learning: In Go packages, variables defined in one file within a package (like `latencyBuckets` and `constLabels` in go/pkg/prometheus/metrics/http.go) are accessible from other files in the same package without requiring imports. This is a common pattern for sharing configuration across multiple files within a package.
Learnt from: chronark
PR: unkeyed/unkey#3560
File: go/deploy/metald/internal/database/repository.go:0-0
Timestamp: 2025-07-15T14:59:30.212Z
Learning: go/deploy/metald cannot currently import helpers from go/pkg/db because it is not yet part of the main Go module; avoid suggesting such imports until the modules are unified.
Learnt from: chronark
PR: unkeyed/unkey#3560
File: go/apps/api/routes/v2_keys_create_key/handler.go:353-466
Timestamp: 2025-07-15T14:25:05.608Z
Learning: In the Unkey codebase, input validation for API endpoints is handled at the OpenAPI schema layer, which validates request fields like permission slugs (pattern: "^[a-zA-Z0-9_]+$", length: 1-100 characters) before requests reach the handler code. This validation occurs during the zen.BindBody call in handlers.
Learnt from: Flo4604
PR: unkeyed/unkey#2955
File: go/apps/api/routes/v2_identities_create_identity/handler.go:162-202
Timestamp: 2025-03-19T09:25:59.751Z
Learning: In the Unkey codebase, input validation for API endpoints is primarily handled through OpenAPI schema validation, which occurs before requests reach the handler code. For example, in the identities.createIdentity endpoint, minimum values for ratelimit duration and limit are defined in the OpenAPI schema rather than duplicating these checks in the handler.
go/apps/api/run.go (1)
Learnt from: chronark
PR: unkeyed/unkey#2901
File: go/pkg/otel/metrics/metrics.go:11-22
Timestamp: 2025-02-26T15:07:05.646Z
Learning: In the metrics package init function, panicking on initialization errors is acceptable since it occurs during startup and indicates a fundamental issue that should be addressed immediately rather than allowing the application to continue with incorrect metrics setup.
🧬 Code Graph Analysis (7)
go/apps/api/config.go (1)
go/pkg/listener/listener.go (1)
Listener(25-79)
go/cmd/api/main.go (2)
go/pkg/listener/port.go (1)
FromPort(72-77)go/pkg/listener/listener.go (1)
Listener(25-79)
go/apps/api/cancel_test.go (2)
go/pkg/listener/ephemeral.go (1)
Ephemeral(64-70)go/pkg/listener/listener.go (1)
Listener(25-79)
go/pkg/zen/server.go (1)
go/pkg/listener/listener.go (1)
Listener(25-79)
go/apps/api/run.go (1)
go/pkg/listener/listener.go (1)
Listener(25-79)
go/pkg/listener/ephemeral.go (1)
go/pkg/listener/listener.go (1)
Listener(25-79)
go/pkg/listener/port.go (1)
go/pkg/listener/listener.go (1)
Listener(25-79)
🪛 markdownlint-cli2 (0.17.2)
go/pkg/zen/README.md
41-41: Hard tabs
Column: 1
(MD010, no-hard-tabs)
145-145: Hard tabs
Column: 1
(MD010, no-hard-tabs)
146-146: Hard tabs
Column: 1
(MD010, no-hard-tabs)
147-147: Hard tabs
Column: 1
(MD010, no-hard-tabs)
148-148: Hard tabs
Column: 1
(MD010, no-hard-tabs)
149-149: Hard tabs
Column: 1
(MD010, no-hard-tabs)
150-150: Hard tabs
Column: 1
(MD010, no-hard-tabs)
151-151: Hard tabs
Column: 1
(MD010, no-hard-tabs)
152-152: Hard tabs
Column: 1
(MD010, no-hard-tabs)
169-169: Hard tabs
Column: 1
(MD010, no-hard-tabs)
196-196: Hard tabs
Column: 1
(MD010, no-hard-tabs)
197-197: 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)
203-203: Hard tabs
Column: 1
(MD010, no-hard-tabs)
204-204: Hard tabs
Column: 1
(MD010, no-hard-tabs)
227-227: Hard tabs
Column: 1
(MD010, no-hard-tabs)
231-231: Hard tabs
Column: 1
(MD010, no-hard-tabs)
275-275: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🪛 LanguageTool
go/pkg/zen/README.md
[grammar] ~217-~217: Use correct spacing
Context: ...utdown } ``` ## Testing with Ephemeral Ports For testing, you can use ephemeral ports...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~219-~219: Use correct spacing
Context: ...This prevents port conflicts in testing environments: go import "github.com/unkeyed/unkey/go/pkg/listener" // Get an available port and listener listenerImpl, err := listener.Ephemeral() if err != nil { t.Fatalf("failed to create ephemeral listener: %v", err) } netListener, err := listenerImpl.Listen() if err != nil { t.Fatalf("failed to get listener: %v", err) } // Start the server go server.Serve(ctx, netListener) // Make requests to the server resp, err := http.Get(fmt.Sprintf("http://%s/test", listenerImpl.Addr())) This approach is especially useful for c...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~241-~241: Use correct spacing
Context: ... run simultaneously without conflicting ports. ## Working with OpenAPI Validation Zen wor...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
🪛 ast-grep (0.38.6)
go/pkg/listener/ephemeral.go
[warning] 64-64: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":0")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control
(avoid-bind-to-all-interfaces-go)
⏰ 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). (5)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
- GitHub Check: Build / Build
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test Packages / Test
🔇 Additional comments (31)
go/pkg/zen/doc.go (1)
52-57: Documentation correctly updated to reflect new listener-based API.The example now properly demonstrates the new pattern where listeners are created explicitly and passed to
server.Serve()instead of using the oldserver.Listen()method. The addition of error handling for listener creation is also appropriate.go/apps/api/config.go (2)
5-5: Import addition looks good.The
listenerpackage import is correctly added to support the new listener abstraction.
27-33: Well-designed configuration field addition.The new
Listenerfield provides a clean abstraction for network listeners while maintaining backward compatibility with the existingHttpPortfield. The documentation clearly explains the precedence and usage patterns.go/cmd/api/main.go (3)
8-8: Import addition is appropriate.The
listenerpackage import is correctly added to support the new listener abstraction in the main command.
208-209: Listener creation follows the new pattern correctly.The code properly creates a listener from the HTTP port using
listener.FromPort(), which aligns with the new listener-based architecture.
237-240: Configuration setup maintains backward compatibility.Both
HttpPortandListenerfields are set appropriately, ensuring backward compatibility while enabling the new listener-based approach.go/apps/api/cancel_test.go (4)
12-12: Import addition is correct.The
listenerpackage import is properly added to support ephemeral listeners in tests.
27-29: Ephemeral listener creation is well-implemented.The test now uses
listener.Ephemeral()instead of manual port allocation, which eliminates race conditions and improves test reliability. Error handling is appropriately added.
41-41: Configuration correctly uses the new listener field.The listener is properly assigned to the config's
Listenerfield, following the new pattern established in the API package.
69-69: HTTP requests correctly use listener address.The test properly uses
listenerImpl.Addr()for both the health check and final verification, ensuring requests target the actual bound address.Also applies to: 94-94
go/apps/api/integration/harness.go (6)
15-15: Import addition is appropriate.The
listenerpackage import is correctly added to support ephemeral listeners in the integration harness.
125-127: Ephemeral listener creation improves test reliability.Using
listener.Ephemeral()for each API node eliminates port allocation race conditions and improves integration test reliability. The error handling is appropriate.
129-129: Cluster address correctly uses listener address.The cluster address is properly constructed using the listener's actual bound address, ensuring accurate communication endpoints.
140-140: API configuration correctly uses listener field.The listener is properly assigned to the API config's
Listenerfield, following the established pattern throughout the codebase.
199-199: Health checks and logging use correct listener address.Both the health check URL and logging statements properly use
listenerImpl.Addr()to target the actual bound address.Also applies to: 205-205
218-220: Good documentation about listener lifecycle management.The comment correctly explains that the listener should not be closed explicitly since the zen server handles graceful shutdown and listener cleanup.
go/pkg/zen/README.md (2)
217-242: Excellent addition: ephemeral port testing documentation.The new "Testing with Ephemeral Ports" section provides clear, practical guidance for using the listener abstraction in tests. The example demonstrates proper resource management with defer cleanup and shows the complete workflow from listener creation to server startup.
145-156: API changes correctly documented.The main quickstart example properly demonstrates the new explicit listener creation pattern, showing error handling for listener creation and the updated server.Serve() method signature.
go/pkg/zen/server.go (2)
166-166: Method signature change aligns with Go conventions.The rename from
ListentoServeand the parameter change fromaddr stringtoln net.Listenerfollows Go's standard library conventions (similar tohttp.Server.Serve). This eliminates the race condition between port allocation and binding.
200-209: Proper usage of provided listener for both HTTP and HTTPS.The implementation correctly uses the provided listener for both TLS and non-TLS scenarios, calling
srv.ServeTLS(ln, "", "")andsrv.Serve(ln)respectively. The logging statements appropriately useln.Addr().String()to report the actual bound address.go/apps/api/run.go (2)
114-123: Prometheus server correctly updated to use explicit listener.The Prometheus server startup now properly creates a TCP listener and passes it to
prom.Serve(), eliminating potential race conditions. The panic on listener creation failure is appropriate for startup errors.
232-257: Flexible listener configuration with proper fallback.The implementation correctly prioritizes
cfg.Listenerwhen provided, falling back to creating a listener fromcfg.HttpPort(defaulting to 7070). This supports both the new listener abstraction and maintains backward compatibility with port-based configuration.go/pkg/listener/ephemeral.go (3)
64-70: Static analysis warning is a false positive.The static analysis tool flagged binding to ":0" as potentially exposing the server publicly. However, this is the standard and correct approach for ephemeral port allocation in testing scenarios. The ":0" address tells the OS to assign an available port, which is exactly what's needed for eliminating race conditions in concurrent tests.
22-24: Excellent design: immediate binding eliminates race conditions.The decision to bind immediately during construction rather than deferring until
Listen()is called is a key design choice that eliminates time-of-check-to-time-of-use race conditions. This makes the implementation ideal for concurrent testing scenarios.Also applies to: 64-70
154-159: Robust Close() implementation with proper nil checking.The
Close()method correctly handles multiple calls and includes nil checking. The documentation appropriately warns about lifecycle management when used with server frameworks.go/pkg/zen/server_tls_test.go (3)
101-108: Tests correctly updated to use ephemeral listener abstraction.The tests now properly use
listener.Ephemeral()instead of directnet.Listen()calls, providing better isolation and eliminating potential port conflicts in concurrent test execution.
121-124: Server method calls and error messages updated consistently.Both test functions correctly call
server.Serve(serverCtx, ln)instead of the oldserver.Listen()method, with error messages updated to reflect the method name change.Also applies to: 221-224
198-205: Consistent application of listener pattern across all tests.The
TestServerWithTLSContextCancellationtest properly implements the same listener pattern as the other test, ensuring consistency in the test suite.go/pkg/listener/listener.go (1)
1-79: Well-designed interface with clear separation of concernsThe
Listenerinterface is thoughtfully designed with a minimal API surface that cleanly separates port allocation strategy from the actual binding operation. The comprehensive documentation clearly explains the rationale, use cases, and thread-safety requirements.go/pkg/listener/port.go (1)
32-77: Constructor validation and design are appropriateThe
FromPortconstructor properly validates port ranges and follows Go conventions by panicking for invalid arguments. The deferred binding pattern is well-suited for production use cases.go/pkg/listener/doc.go (1)
1-117: Excellent package documentationThe package documentation is comprehensive, well-structured, and provides clear examples for both production and testing scenarios. The architectural rationale and design decisions are well-explained.
go/pkg/listener/port.go
Outdated
| func (p *PortListener) Addr() string { | ||
| if p.listener != nil { | ||
| return p.listener.Addr().String() | ||
| } | ||
| return fmt.Sprintf(":%d", p.port) | ||
| } |
There was a problem hiding this comment.
Potential data race in Addr() method
The Addr() method reads p.listener without synchronization, which could race with a concurrent Listen() call that's writing to it. While pointer reads/writes are atomic on most architectures, this is still technically a data race in Go.
This should be protected by the same mutex suggested for Listen():
func (p *PortListener) Addr() string {
+ p.mu.Lock()
+ defer p.mu.Unlock()
+
if p.listener != nil {
return p.listener.Addr().String()
}
return fmt.Sprintf(":%d", p.port)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (p *PortListener) Addr() string { | |
| if p.listener != nil { | |
| return p.listener.Addr().String() | |
| } | |
| return fmt.Sprintf(":%d", p.port) | |
| } | |
| func (p *PortListener) Addr() string { | |
| p.mu.Lock() | |
| defer p.mu.Unlock() | |
| if p.listener != nil { | |
| return p.listener.Addr().String() | |
| } | |
| return fmt.Sprintf(":%d", p.port) | |
| } |
🤖 Prompt for AI Agents
In go/pkg/listener/port.go around lines 144 to 149, the Addr() method accesses
p.listener without synchronization, causing a potential data race with
concurrent writes in Listen(). To fix this, protect the access to p.listener in
Addr() by acquiring the same mutex used in Listen() before reading p.listener,
and release it afterward to ensure safe concurrent access.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
go/apps/api/cancel_test.go(5 hunks)go/apps/api/config.go(2 hunks)go/apps/api/integration/harness.go(6 hunks)go/apps/api/run.go(3 hunks)go/cmd/api/main.go(3 hunks)go/pkg/zen/server_tls_test.go(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
go/apps/api/integration/harness.go (2)
Learnt from: Flo4604
PR: unkeyed/unkey#3606
File: go/pkg/prometheus/metrics/database.go:29-30
Timestamp: 2025-07-16T10:06:35.397Z
Learning: In Go packages, variables defined in one file within a package (like `latencyBuckets` and `constLabels` in go/pkg/prometheus/metrics/http.go) are accessible from other files in the same package without requiring imports. This is a common pattern for sharing configuration across multiple files within a package.
Learnt from: chronark
PR: unkeyed/unkey#3560
File: go/deploy/metald/internal/database/repository.go:0-0
Timestamp: 2025-07-15T14:59:30.212Z
Learning: go/deploy/metald cannot currently import helpers from go/pkg/db because it is not yet part of the main Go module; avoid suggesting such imports until the modules are unified.
go/apps/api/run.go (1)
Learnt from: chronark
PR: unkeyed/unkey#2901
File: go/pkg/otel/metrics/metrics.go:11-22
Timestamp: 2025-02-26T15:07:05.646Z
Learning: In the metrics package init function, panicking on initialization errors is acceptable since it occurs during startup and indicates a fundamental issue that should be addressed immediately rather than allowing the application to continue with incorrect metrics setup.
🧬 Code Graph Analysis (3)
go/cmd/api/main.go (2)
go/pkg/listener/port.go (1)
FromPort(72-77)go/pkg/listener/listener.go (1)
Listener(25-79)
go/apps/api/config.go (1)
go/pkg/listener/listener.go (1)
Listener(25-79)
go/apps/api/integration/harness.go (1)
go/pkg/listener/listener.go (1)
Listener(25-79)
🪛 ast-grep (0.38.6)
go/apps/api/cancel_test.go
[warning] 27-27: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":0")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control
(avoid-bind-to-all-interfaces-go)
go/apps/api/integration/harness.go
[warning] 126-126: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":0")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control
(avoid-bind-to-all-interfaces-go)
go/pkg/zen/server_tls_test.go
[warning] 101-101: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":0")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control
(avoid-bind-to-all-interfaces-go)
[warning] 196-196: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":0")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control
(avoid-bind-to-all-interfaces-go)
⏰ 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). (7)
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test Packages / Test
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (20)
go/pkg/zen/server_tls_test.go (5)
20-20: LGTM: Import updated for listener package integration.The import change aligns with the broader refactoring to use the listener package abstraction.
101-106: LGTM: Ephemeral listener creation is correct for tests.The direct use of
net.Listen("tcp", ":0")for ephemeral ports is appropriate for test isolation and avoids port conflicts. The static analysis warning about binding to all interfaces is a false positive in this testing context - ephemeral listeners are intended for isolated test environments.
119-122: LGTM: Server API transition handled correctly.The change from
server.Listen(addr)toserver.Serve(ctx, listener)properly implements the new listener-based approach. Error handling and messaging are appropriately updated.
196-201: LGTM: Consistent listener pattern implementation.The second test function correctly follows the same ephemeral listener pattern, maintaining consistency across the test suite.
217-220: LGTM: Server API usage is consistent.The server startup logic properly uses the new
Servemethod with the listener parameter.go/cmd/api/main.go (1)
8-8: LGTM: Listener package import added.The import supports the new listener abstraction being introduced across the codebase.
go/apps/api/config.go (2)
4-7: LGTM: Imports added for listener functionality.The new imports support the listener abstraction and provide the necessary types for the Config struct.
29-35: LGTM: Well-designed listener field addition.The new
Listenerfield is properly documented with clear precedence rules and use cases. The documentation explains that it's intended for testing scenarios requiring ephemeral ports, while production uses theHttpPortfield. This provides good flexibility for different deployment scenarios.go/apps/api/cancel_test.go (4)
6-6: LGTM: Import updated for direct listener usage.The change from the port package to the standard
netpackage aligns with the new approach of creating ephemeral listeners directly.
27-30: LGTM: Ephemeral listener creation improves test isolation.Creating ephemeral listeners directly with
net.Listen("tcp", ":0")eliminates potential port conflicts and race conditions. The static analysis warning about binding to all interfaces is a false positive in this testing context.
41-41: LGTM: Config properly uses listener field.The configuration correctly uses the new
Listenerfield instead of relying on port numbers, which aligns with the architectural changes.
69-69: LGTM: HTTP requests use listener address correctly.The test properly uses
ln.Addr()to construct HTTP request URLs, ensuring requests are directed to the actual bound address.Also applies to: 94-94
go/apps/api/integration/harness.go (5)
6-6: LGTM: Imports updated for listener functionality.The addition of the
netpackage andlistenerpackage imports supports the new ephemeral listener approach.Also applies to: 16-16
126-131: LGTM: Ephemeral listener creation eliminates port conflicts.Creating ephemeral listeners for each API node prevents port conflicts during concurrent testing. The use of
ln.Addr().String()correctly extracts the actual bound address. The static analysis warning about binding to all interfaces is a false positive in this testing context.
141-141: LGTM: Config uses listener field properly.The API configuration correctly uses the new
Listenerfield, aligning with the architectural changes.
200-206: LGTM: Health checks use listener address correctly.The health check logic properly constructs URLs using the listener's address, ensuring accurate connectivity verification.
219-221: LGTM: Resource cleanup strategy is appropriate.The comment correctly notes that the zen server handles listener closure during graceful shutdown, avoiding double-close issues. This is a good resource management approach.
go/apps/api/run.go (3)
7-7: LGTM: Import addition is correct.The
"net"import is properly added to support the new listener creation functionality.
114-118: LGTM: Prometheus listener creation follows the new pattern correctly.The explicit listener creation and error handling are consistent with the new listener-based approach. The use of
panic(err)for listener creation failure is appropriate during server startup, as it indicates a fundamental issue that should prevent the application from continuing.
230-236: LGTM: Well-designed conditional listener creation.The conditional listener creation provides excellent flexibility - supporting both injected listeners (for testing) and port-based listeners (for production). The error handling is appropriate and the error message is descriptive.
we now try to listen on a random port assigned by the OS and never stop listening until the tests are done. This prevents the race conditions between assigning a port and using it
chronark
force-pushed
the
07-17-fix_port_allocation_in_tests
branch
from
96638e7 to
c3fa9b5
Compare
|
|
Graphite Automations"Post a GIF when PR approved" took an action on this PR • (07/17/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
* begining of things * Stop crashing logs * fix the ability to retrieve from CH * Fix all bugs * formatting * Better logs, fix some errors, typesafe * update the v1_keys_getVerifications * fmt * delete all tests --------- Co-authored-by: MichaelUnkey <michael@unkey.com>
chronark
requested review from
MichaelUnkey,
mcstepp and
ogzhanolguncu
as code owners
* feat: add commands * feat: allow configuring name,desc and version * feat: pass env to cli * feat: match the initial impl * feat: add new progress aniamtion * feat: add tracker step for each phase * refactor: improve animations and errors * feat: use proper orchestrafor managing steps and trackers * refactor: rename build to run * refactor: remove UI logic from api * chore: remove redundant commands * refactor: remove ui bloat * feat: add colors for make it distinguishable * fix: steps * fix: code rabbit issues * feat: add proper flag parsing logic * refactor: show help if required args are missing * feat: add missing commands * fix: code rabbit comments * refactor: fix redundancy * refactor: improve sub spinner * refactor: move duplicated spinner loop * refactor: remove some commands for later
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/job_test_api_local.yaml(1 hunks)
⏰ 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). (5)
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test Go API Local / Test
- GitHub Check: Build / Build
- GitHub Check: Test Packages / Test
- GitHub Check: Analyze (javascript-typescript)
| - name: Run containers | ||
| run: docker compose -f ./deployment/docker-compose.yaml up mysql planetscale agent s3 chproxy api -d | ||
| run: docker compose -f ./deployment/docker-compose.yaml up -d | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Block until containers are healthy to avoid race conditions
docker compose up -d returns immediately, so the subsequent “Install / Build / DB migrations” steps may run before MySQL, ClickHouse, or the API container finish their health-checks. That re-introduces the very race conditions this PR is trying to eliminate.
A minimal change is to leverage Compose v2’s built-in --wait flag (available on the GitHub runner) so the workflow blocks until all services report healthy, with an explicit timeout:
-run: docker compose -f ./deployment/docker-compose.yaml up -d
+run: docker compose -f ./deployment/docker-compose.yaml up -d --wait --wait-timeout 120If the installed Compose plugin is older and lacks --wait, add a dedicated wait step (e.g. polling docker compose ps --format json for State.Health.Status=="healthy").
Please verify the runner’s docker compose version supports --wait; otherwise the job will fail at startup.
🌐 Web query:
When was the --wait flag introduced in Docker Compose?
💡 Result:
The --wait flag was introduced in Docker Compose version 2.1.1. This flag allows you to run docker-compose up -d --wait, making Docker Compose wait until all containers are healthy before the command exits (while containers continue running in the background). To use --wait, each relevant container must have a healthcheck defined in the docker-compose.yml file[1].
“Docker Compose since version 2.1.1 has a new flag for the detached mode –
--wait.”[1]
Citations:
- 1: https://maciejwalkowiak.com/blog/docker-compose-waiting-containers-ready/
- 2: https://docs.docker.com/reference/cli/docker/container/wait/
- 3: Add timeout to --wait option docker/compose#10269
- 4: https://github.com/ufoscout/docker-compose-wait
- 5: https://labex.io/tutorials/docker-how-to-use-docker-compose-wait-command-to-block-until-services-stop-555097
- 6: Feature: docker-compose up -d with waiting for healthy statuses docker/compose#8351
- 7: https://dev.to/welel/wait-for-services-to-start-in-docker-compose-wait-for-it-vs-healthcheck-127d
- 8: How can I wait for a container to exit? docker/compose#5007
- 9: https://www.okteto.com/docs/reference/docker-compose/
- 10: https://github.com/peter-evans/docker-compose-healthcheck
🌐 Web query:
What version of Docker Compose is available on GitHub Actions ubuntu-latest runner?
💡 Result:
As of July 17, 2025, the ubuntu-latest runner in GitHub Actions includes Docker Compose v2. This change aligns with GitHub's deprecation of Docker Compose v1, which was removed from their hosted runner images on July 9, 2024. (github.blog)
To utilize Docker Compose in your GitHub Actions workflows, you can use the docker/setup-compose-action to ensure the latest version is installed:
name: CI
on:
push:
branches: [ "main" ]
jobs:
build:
runs-on: ubuntu-latest
steps:
- name: Set up Docker Compose
uses: docker/setup-compose-action@v1
with:
version: latestThis action will install the latest stable version of Docker Compose, ensuring compatibility with your workflows. (github.com)
For more details on the deprecation of Docker Compose v1 and the transition to v2, refer to GitHub's official announcement. (github.blog)
Citations:
- 1: https://github.blog/changelog/2024-04-10-github-hosted-runner-images-deprecation-notice-docker-compose-v1/?utm_source=openai
- 2: https://github.com/marketplace/actions/docker-setup-compose?utm_source=openai
- 3: https://github.blog/changelog/2024-04-10-github-hosted-runner-images-deprecation-notice-docker-compose-v1/?utm_source=openai
Ensure containers are healthy before proceeding with --wait
Docker Compose v2 (≥2.1.1) on Ubuntu-latest supports the --wait flag, so you can block until all services report healthy. Update your workflow as follows:
• File: .github/workflows/job_test_api_local.yaml (lines 17–19)
- - name: Run containers
- run: docker compose -f ./deployment/docker-compose.yaml up -d
+ - name: Run containers
+ run: docker compose -f ./deployment/docker-compose.yaml up -d --wait --wait-timeout 120If you need to guarantee the installed Compose version or pin it explicitly, add before this step:
- name: Set up Docker Compose
uses: docker/setup-compose-action@v1
with:
version: latestThis ensures your “Install / Build / DB migrations” steps won’t start until MySQL, ClickHouse, and the API are healthy.
🤖 Prompt for AI Agents
In .github/workflows/job_test_api_local.yaml around lines 17 to 19, update the
"Run containers" step to include the `--wait` flag with `docker compose` to
ensure the workflow waits until all services are healthy before proceeding.
Additionally, add a step before this to set up Docker Compose explicitly using
the docker/setup-compose-action@v1 with the latest version to guarantee
compatibility with the `--wait` flag.
we now try to listen on a random port assigned by the OS and never stop listening until the tests are done. This prevents the race conditions between assigning a port and using it
…om/unkeyed/unkey into 07-17-fix_port_allocation_in_tests
There was a problem hiding this comment.
Actionable comments posted: 22
🔭 Outside diff range comments (2)
apps/agent/cmd/agent/agent.go (1)
153-155: Remove dead code blockThis error check is redundant as
errwill always benilat this point. The vault creation error is already handled at lines 149-151.- if err != nil { - return fmt.Errorf("failed to create vault service: %w", err) - }apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/hooks/use-filters.ts (1)
118-156: Validation for other fields is inconsistent with tags validation.The validation for outcomes, time fields, and since uses the
operatorvariable (which defaults to the first valid operator) but still throws errors. This could lead to unreachable error cases sinceoperatoris already validated.Consider either:
- Removing the error throws since
operatoris pre-validated- Using
filter.operatorin the conditions instead ofoperator
| if opts.DockerImage == "" { | ||
| if !isDockerAvailable() { | ||
| ui.PrintError("Docker not found - please install Docker") | ||
| ui.PrintErrorDetails(ErrDockerNotFound.Error()) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider moving Docker availability check earlier.
The Docker availability check is only performed when DockerImage is empty, but Docker might be required for other operations (e.g., if someone provides a local image that needs to be tagged/pushed). Consider moving this check to the beginning of the function if Docker operations are expected.
🤖 Prompt for AI Agents
In go/cmd/cli/commands/deploy/deploy.go around lines 141 to 146, the Docker
availability check is currently only done when opts.DockerImage is empty, but
Docker might be needed for other operations regardless. Move the Docker
availability check to the start of the function to ensure Docker is available
before any Docker-related operations, including tagging or pushing local images,
are performed.
| // Step predictor - maps current step message patterns to next expected steps | ||
| // Based on the actual workflow messages from version.go | ||
| // TODO: In the future directly get those from hydra | ||
| var stepSequence = map[string]string{ | ||
| "Version queued and ready to start": "Downloading Docker image:", | ||
| "Downloading Docker image:": "Building rootfs from Docker image:", | ||
| "Building rootfs from Docker image:": "Uploading rootfs image to storage", | ||
| "Uploading rootfs image to storage": "Creating VM for version:", | ||
| "Creating VM for version:": "VM booted successfully:", | ||
| "VM booted successfully:": "Assigned hostname:", | ||
| "Assigned hostname:": "Version deployment completed successfully", | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
🛠️ Refactor suggestion
Consider using structured step identifiers instead of message-based mapping.
The current approach of mapping step messages to predict next steps is fragile and will break if message formats change. Consider using enum-based step identifiers or a structured step definition that separates the display message from the step ID.
For example:
type DeploymentStep string
const (
StepQueued DeploymentStep = "queued"
StepDownloading DeploymentStep = "downloading"
StepBuildingRootfs DeploymentStep = "building_rootfs"
// etc...
)
type StepDefinition struct {
ID DeploymentStep
Message string
NextStep DeploymentStep
}This would make the workflow more maintainable and less prone to breaking when messages are updated.
🤖 Prompt for AI Agents
In go/cmd/cli/commands/deploy/deploy.go around lines 14 to 25, replace the
current map that uses step message strings as keys and values with a more robust
structure using enum-like step identifiers. Define a DeploymentStep type with
constants for each step ID, then create a StepDefinition struct containing the
step ID, display message, and next step ID. Refactor the stepSequence map to use
these structured step definitions instead of raw message strings to improve
maintainability and reduce fragility from message format changes.
| ui.PrintErrorDetails(err.Error()) | ||
| // INFO: For now we are ignoring registry push because everyone one is working locally, | ||
| // omit this when comments and put the return nil back when going to prod | ||
| // return nil |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix typo in comment.
- // INFO: For now we are ignoring registry push because everyone one is working locally,
+ // INFO: For now we are ignoring registry push because everyone is working locally,Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In go/cmd/cli/commands/deploy/deploy.go at line 169, there is a typo in the
comment "// return nil". Correct the comment to fix the typo, ensuring it
accurately reflects the intended message or action.
| startTime: timeConfig.startTime, | ||
| keyIds: [], | ||
| names: [], | ||
| tags: null, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify initialization pattern for tags field.
The tags field is initialized to null while other array fields (identities, keyIds, names) are initialized to empty arrays []. Ensure this initialization pattern aligns with the expected VerificationTimeseriesParams type definition.
🏁 Script executed:
#!/bin/bash
# Check the VerificationTimeseriesParams type definition to verify the expected type for tags
ast-grep --pattern 'export type VerificationTimeseriesParams = $_'Length of output: 202
To verify how tags is defined in the Zod schema, let’s inspect the verificationTimeseriesParams declaration:
🏁 Script executed:
#!/bin/bash
# Locate and show the Zod schema for verificationTimeseriesParams
rg -C5 "const verificationTimeseriesParams" -n internal/clickhouse/src/verifications.tsLength of output: 362
Let’s inspect the full Zod definition to see how tags is typed:
🏁 Script executed:
#!/bin/bash
# Show verificationTimeseriesParams schema including tags definition
rg -A50 'export const verificationTimeseriesParams' -n internal/clickhouse/src/verifications.tsLength of output: 1431
Let’s inspect the surrounding logic in utils.ts to see how each filter—including tags—is transformed:
🏁 Script executed:
#!/bin/bash
# Show the full transform function to compare default values for all filters
rg -C10 "export function" -n apps/dashboard/lib/trpc/routers/api/overview/query-timeseries/utils.ts
# Specifically print lines around the `tags:` assignment
rg -C5 "tags:" -n apps/dashboard/lib/trpc/routers/api/overview/query-timeseries/utils.tsLength of output: 1411
Align default filter initializations
In apps/dashboard/lib/trpc/routers/api/overview/query-timeseries/utils.ts, the transform defaults most filter arrays to [] but initializes tags to null. Since the Zod schema defines all of these fields as array(...).nullable(), it’s best to keep the pattern consistent. Either:
- Default all filters to
null, or - Default all filters to
[]
Likely the simplest fix is to initialize tags as an empty array:
• File: apps/dashboard/lib/trpc/routers/api/overview/query-timeseries/utils.ts
Line ~31
- tags: null,
+ tags: [],🤖 Prompt for AI Agents
In apps/dashboard/lib/trpc/routers/api/overview/query-timeseries/utils.ts at
line 31, the default initialization of the 'tags' filter is set to null while
other filter arrays default to empty arrays. To maintain consistency with the
Zod schema and other filters, change the default value of 'tags' from null to an
empty array ([]).
| tags: z | ||
| .object({ | ||
| operator: keysOverviewFilterOperatorEnum, | ||
| value: z.string(), | ||
| }) | ||
| .nullable(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider schema consistency with other filter fields.
The tags field structure differs from other filter fields (keyIds, names, identities) which use arrays of filter objects. This inconsistency may limit the ability to apply multiple tag filters simultaneously.
Consider aligning the tags field structure with other filter fields:
- tags: z
- .object({
- operator: keysOverviewFilterOperatorEnum,
- value: z.string(),
- })
- .nullable(),
+ tags: z
+ .object({
+ filters: z.array(
+ z.object({
+ operator: keysOverviewFilterOperatorEnum,
+ value: z.string(),
+ }),
+ ),
+ })
+ .nullable(),This would maintain consistency with the existing filter pattern and allow multiple tag filters if needed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tags: z | |
| .object({ | |
| operator: keysOverviewFilterOperatorEnum, | |
| value: z.string(), | |
| }) | |
| .nullable(), | |
| tags: z | |
| .object({ | |
| filters: z.array( | |
| z.object({ | |
| operator: keysOverviewFilterOperatorEnum, | |
| value: z.string(), | |
| }), | |
| ), | |
| }) | |
| .nullable(), |
🤖 Prompt for AI Agents
In
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/charts/bar-chart/query-timeseries.schema.ts
around lines 53 to 58, the tags field is defined as a single nullable filter
object, unlike other filter fields such as keyIds, names, and identities which
use arrays of filter objects. To fix this, change the tags field to be an array
of filter objects (each with operator and value) instead of a single object,
ensuring it matches the pattern used by other filters and supports multiple tag
filters simultaneously.
| // Creation timestamp as unique identifier | ||
| stepTimestamp := step.GetCreatedAt() | ||
|
|
||
| if processedSteps[stepTimestamp] { | ||
| continue // Already processed this step | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve step identification to avoid potential collisions.
Using creation timestamp as the unique identifier could lead to collisions if multiple steps are created at the exact same millisecond. Consider using a combination of fields or requesting a unique step ID from the API.
- // Creation timestamp as unique identifier
- stepTimestamp := step.GetCreatedAt()
-
- if processedSteps[stepTimestamp] {
+ // Use combination of timestamp and message for uniqueness
+ stepKey := fmt.Sprintf("%d:%s", step.GetCreatedAt(), step.GetMessage())
+
+ if processedSteps[stepKey] {
continue // Already processed this step
}Also update the map declaration in PollVersionStatus:
- processedSteps := make(map[int64]bool)
+ processedSteps := make(map[string]bool)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In go/cmd/cli/commands/deploy/control_plane.go around lines 162 to 168, the code
uses the step creation timestamp as a unique identifier, which can cause
collisions if multiple steps share the same timestamp. To fix this, modify the
identifier to combine multiple fields from the step (such as step ID, name, or
other unique attributes) or use a unique step ID provided by the API if
available. Also, update the processedSteps map declaration in PollVersionStatus
to use the new identifier type accordingly.
| // PollVersionStatus polls for version changes and calls event handlers | ||
| func (c *ControlPlaneClient) PollVersionStatus( | ||
| ctx context.Context, | ||
| logger logging.Logger, | ||
| versionId string, | ||
| onStatusChange func(VersionStatusEvent) error, | ||
| onStepUpdate func(VersionStepEvent) error, | ||
| ) error { | ||
| ticker := time.NewTicker(2 * time.Second) | ||
| defer ticker.Stop() | ||
| timeout := time.NewTimer(300 * time.Second) | ||
| defer timeout.Stop() | ||
|
|
||
| // Track processed steps by creation time to avoid duplicates | ||
| processedSteps := make(map[int64]bool) | ||
| lastStatus := ctrlv1.VersionStatus_VERSION_STATUS_UNSPECIFIED | ||
|
|
||
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return ctx.Err() | ||
| case <-timeout.C: | ||
| return fmt.Errorf("deployment timeout after 5 minutes") | ||
| case <-ticker.C: | ||
| version, err := c.GetVersion(ctx, versionId) | ||
| if err != nil { | ||
| logger.Debug("Failed to get version status", "error", err, "version_id", versionId) | ||
| continue | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improvements to polling logic.
- The 5-minute timeout might be insufficient for large deployments
- Silently continuing on API errors could mask persistent issues
- Consider exponential backoff instead of fixed 2-second intervals
func (c *ControlPlaneClient) PollVersionStatus(
ctx context.Context,
logger logging.Logger,
versionId string,
onStatusChange func(VersionStatusEvent) error,
onStepUpdate func(VersionStepEvent) error,
) error {
- ticker := time.NewTicker(2 * time.Second)
+ // Start with 1 second, max 10 seconds
+ backoff := time.Second
+ maxBackoff := 10 * time.Second
+ ticker := time.NewTicker(backoff)
defer ticker.Stop()
- timeout := time.NewTimer(300 * time.Second)
+ // Make timeout configurable, default to 10 minutes
+ timeoutDuration := 10 * time.Minute
+ if c.opts.DeploymentTimeout > 0 {
+ timeoutDuration = c.opts.DeploymentTimeout
+ }
+ timeout := time.NewTimer(timeoutDuration)
defer timeout.Stop()
+ var consecutiveErrors int
+ maxConsecutiveErrors := 5
+
// Track processed steps by creation time to avoid duplicates
processedSteps := make(map[int64]bool)
lastStatus := ctrlv1.VersionStatus_VERSION_STATUS_UNSPECIFIED
for {
select {
case <-ctx.Done():
return ctx.Err()
case <-timeout.C:
- return fmt.Errorf("deployment timeout after 5 minutes")
+ return fmt.Errorf("deployment timeout after %v", timeoutDuration)
case <-ticker.C:
version, err := c.GetVersion(ctx, versionId)
if err != nil {
logger.Debug("Failed to get version status", "error", err, "version_id", versionId)
+ consecutiveErrors++
+ if consecutiveErrors >= maxConsecutiveErrors {
+ return fmt.Errorf("failed to get version status after %d attempts: %w", consecutiveErrors, err)
+ }
+ // Exponential backoff on errors
+ backoff = backoff * 2
+ if backoff > maxBackoff {
+ backoff = maxBackoff
+ }
+ ticker.Reset(backoff)
continue
}
+
+ // Reset on success
+ consecutiveErrors = 0
+ backoff = time.Second
+ ticker.Reset(backoff)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In go/cmd/cli/commands/deploy/control_plane.go between lines 93 and 121, improve
the PollVersionStatus function by increasing or making the timeout configurable
to handle large deployments, replace the fixed 2-second ticker with an
exponential backoff mechanism to reduce load and improve efficiency, and modify
the error handling on API call failures to avoid silently continuing by logging
errors more prominently or returning after repeated failures to surface
persistent issues.
| // NewControlPlaneClient creates a new control plane client | ||
| func NewControlPlaneClient(opts *DeployOptions) *ControlPlaneClient { | ||
| httpClient := &http.Client{} | ||
| client := ctrlv1connect.NewVersionServiceClient(httpClient, opts.ControlPlaneURL) | ||
|
|
||
| return &ControlPlaneClient{ | ||
| client: client, | ||
| opts: opts, | ||
| } | ||
| } |
There was a problem hiding this comment.
Configure HTTP client timeout to prevent hanging requests.
The HTTP client is created without any timeout configuration, which could lead to indefinitely hanging requests if the control plane becomes unresponsive.
func NewControlPlaneClient(opts *DeployOptions) *ControlPlaneClient {
- httpClient := &http.Client{}
+ httpClient := &http.Client{
+ Timeout: 30 * time.Second,
+ }
client := ctrlv1connect.NewVersionServiceClient(httpClient, opts.ControlPlaneURL)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // NewControlPlaneClient creates a new control plane client | |
| func NewControlPlaneClient(opts *DeployOptions) *ControlPlaneClient { | |
| httpClient := &http.Client{} | |
| client := ctrlv1connect.NewVersionServiceClient(httpClient, opts.ControlPlaneURL) | |
| return &ControlPlaneClient{ | |
| client: client, | |
| opts: opts, | |
| } | |
| } | |
| func NewControlPlaneClient(opts *DeployOptions) *ControlPlaneClient { | |
| httpClient := &http.Client{ | |
| Timeout: 30 * time.Second, | |
| } | |
| client := ctrlv1connect.NewVersionServiceClient(httpClient, opts.ControlPlaneURL) | |
| return &ControlPlaneClient{ | |
| client: client, | |
| opts: opts, | |
| } | |
| } |
🤖 Prompt for AI Agents
In go/cmd/cli/commands/deploy/control_plane.go around lines 40 to 49, the HTTP
client is created without a timeout, risking indefinite hanging if the control
plane is unresponsive. Fix this by setting a reasonable timeout value (e.g., 10
seconds) on the http.Client instance before passing it to
NewVersionServiceClient.
| // Flag represents a command line flag interface | ||
| // All flag types must implement these methods | ||
| type Flag interface { | ||
| Name() string // The flag name (without dashes) | ||
| Usage() string // Help text describing the flag | ||
| Required() bool // Whether this flag is mandatory | ||
| Parse(value string) error // Parse string value into the flag's type | ||
| IsSet() bool // Whether the flag was explicitly set by user | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add EnvVar() to the Flag interface.
As suggested in the help.go review, adding EnvVar() string to the Flag interface would eliminate the need for type assertions in getEnvVar().
type Flag interface {
Name() string // The flag name (without dashes)
Usage() string // Help text describing the flag
Required() bool // Whether this flag is mandatory
Parse(value string) error // Parse string value into the flag's type
IsSet() bool // Whether the flag was explicitly set by user
+ EnvVar() string // Environment variable name for default value
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Flag represents a command line flag interface | |
| // All flag types must implement these methods | |
| type Flag interface { | |
| Name() string // The flag name (without dashes) | |
| Usage() string // Help text describing the flag | |
| Required() bool // Whether this flag is mandatory | |
| Parse(value string) error // Parse string value into the flag's type | |
| IsSet() bool // Whether the flag was explicitly set by user | |
| } | |
| // Flag represents a command line flag interface | |
| // All flag types must implement these methods | |
| type Flag interface { | |
| Name() string // The flag name (without dashes) | |
| Usage() string // Help text describing the flag | |
| Required() bool // Whether this flag is mandatory | |
| Parse(value string) error // Parse string value into the flag's type | |
| IsSet() bool // Whether the flag was explicitly set by user | |
| EnvVar() string // Environment variable name for default value | |
| } |
🤖 Prompt for AI Agents
In go/cmd/cli/cli/flag.go between lines 10 and 18, the Flag interface lacks the
EnvVar() string method, which is needed to avoid type assertions in getEnvVar().
Add the EnvVar() string method declaration to the Flag interface so all flag
types implement it, enabling direct calls to EnvVar() without type assertions.
| // Check environment variable for default value | ||
| if envVar != "" { | ||
| if envValue := os.Getenv(envVar); envValue != "" { | ||
| if parsed, err := strconv.ParseBool(envValue); err == nil { | ||
| flag.value = parsed | ||
| flag.set = true // Mark as set since env var was found | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Log warnings for invalid environment variable values.
When environment variables contain invalid values (e.g., non-boolean for Bool flag), they are silently ignored. Consider logging warnings to help users debug configuration issues.
// Check environment variable for default value
if envVar != "" {
if envValue := os.Getenv(envVar); envValue != "" {
- if parsed, err := strconv.ParseBool(envValue); err == nil {
+ if parsed, err := strconv.ParseBool(envValue); err != nil {
+ // Consider injecting a logger or using a package-level logger
+ fmt.Fprintf(os.Stderr, "Warning: Invalid boolean value '%s' for environment variable %s\n", envValue, envVar)
+ } else {
flag.value = parsed
flag.set = true // Mark as set since env var was found
}
}
}Apply similar changes to Int and Float constructors.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Check environment variable for default value | |
| if envVar != "" { | |
| if envValue := os.Getenv(envVar); envValue != "" { | |
| if parsed, err := strconv.ParseBool(envValue); err == nil { | |
| flag.value = parsed | |
| flag.set = true // Mark as set since env var was found | |
| } | |
| } | |
| } | |
| // Check environment variable for default value | |
| if envVar != "" { | |
| if envValue := os.Getenv(envVar); envValue != "" { | |
| if parsed, err := strconv.ParseBool(envValue); err != nil { | |
| // Consider injecting a logger or using a package-level logger | |
| fmt.Fprintf(os.Stderr, "Warning: Invalid boolean value '%s' for environment variable %s\n", envValue, envVar) | |
| } else { | |
| flag.value = parsed | |
| flag.set = true // Mark as set since env var was found | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In go/cmd/cli/cli/flag.go around lines 191 to 199, the code silently ignores
invalid environment variable values when parsing booleans. Modify the code to
log a warning message whenever strconv.ParseBool returns an error, indicating
the invalid value and the environment variable name. Apply the same pattern of
logging warnings for invalid environment variable values in the Int and Float
flag constructors to improve user debugging.
3 participants
What does this PR do?
Introduces a new
listenerpackage to replace the existingportpackage, providing a more robust and flexible approach to network port management. This change eliminates race conditions in port allocation and improves testing reliability.Key improvements:
Listenerinterface with implementations for specific ports and ephemeral (OS-assigned) portszen.Serverto useServe(net.Listener)instead ofListen(addr string)Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Bug Fixes
Chores
New Features (CLI)