Adds Faucet + gateway to launcher #1734
Conversation
WalkthroughThe overall change encompasses a significant update to the configuration and management of the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChat with CodeRabbit Bot (
|
| @@ -49,7 +49,6 @@ func TestFaucet(t *testing.T) { | |||
| time.Sleep(2 * time.Second) | |||
|
|
|||
| faucetConfig := &faucet.Config{ | |||
| Port: startPort, | |||
There was a problem hiding this comment.
This flag was unused and threw me off when hooking up the ports
|
|
||
| // returns the remaining native balance of the faucet | ||
| func healthReqHandler() gin.HandlerFunc { | ||
| return func(c *gin.Context) { | ||
| c.JSON(http.StatusOK, gin.H{"healthy": true}) | ||
| } | ||
| } |
There was a problem hiding this comment.
Faucet didn't have a health handler - particularly useful to determine when the container is up and running.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 9
Configuration used: CodeRabbit UI
Files selected for processing (9)
- integration/faucet/faucet_test.go (1 hunks)
- testnet/launcher/docker.go (2 hunks)
- testnet/launcher/faucet/config.go (1 hunks)
- testnet/launcher/faucet/docker.go (1 hunks)
- testnet/launcher/gateway/config.go (1 hunks)
- testnet/launcher/gateway/docker.go (1 hunks)
- tools/faucet/cmd/cli.go (3 hunks)
- tools/faucet/faucet/faucet_config.go (1 hunks)
- tools/faucet/webserver/web_server.go (2 hunks)
Files skipped from review due to trivial changes (2)
- tools/faucet/cmd/cli.go
- tools/faucet/faucet/faucet_config.go
Additional comments: 13
testnet/launcher/faucet/config.go (3)
6-13: The
Configstruct fields are unexported, which may limit the ability to set them directly from outside the package. Ensure this is intentional and that configuration is only intended to be done via the provided option functions.15-23: The
NewFaucetConfigfunction correctly applies the functional options pattern to configure theConfigobject.25-53: The option functions
WithTenNodeHost,WithFaucetPrivKey,WithDockerImage,WithTenNodePort, andWithFaucetPortare correctly implemented following the functional options pattern.testnet/launcher/gateway/config.go (3)
6-14: The
Configstruct fields are unexported, which may limit the ability to set them directly from outside the package. Ensure this is intentional and that configuration is only intended to be done via the provided option functions.16-24: The
NewGatewayConfigfunction correctly applies the functional options pattern to configure theConfigobject.26-60: The option functions
WithTenNodeHost,WithDockerImage,WithTenNodeHTTPPort,WithTenNodeWSPort,WithGatewayHTTPPort, andWithGatewayWSPortare correctly implemented following the functional options pattern.testnet/launcher/faucet/docker.go (1)
- 39-55: The
IsReadymethod correctly implements a retry mechanism to check the readiness of the Docker Faucet service. Ensure that the hardcodedhttp://127.0.0.1address is appropriate for the readiness check in all deployment scenarios.testnet/launcher/gateway/docker.go (1)
- 41-57: The
IsReadymethod correctly implements a retry mechanism to check the readiness of the Docker Gateway service. Ensure that the hardcodedhttp://127.0.0.1address is appropriate for the readiness check in all deployment scenarios.tools/faucet/webserver/web_server.go (1)
- 176-181: The
healthReqHandlerfunction is added to return the health status of the faucet. Ensure that the health status accurately reflects the operational state of the faucet.integration/faucet/faucet_test.go (1)
- 51-54: The
Portdeclaration has been removed from thefaucetConfiginitialization. Verify that the removal of this field does not affect the test's ability to connect to the correct port for the faucet service.Verification successful
The output from the shell script shows that the
faucetConfigstruct infaucet_test.gohas been updated to include aServerPortfield, which is used to get the faucet balance and fund the wallet. This suggests that thePortfield has been replaced withServerPort, and the tests are using this new field to connect to the correct port for the faucet service.Based on this information, it appears that the removal of the
Portfield does not affect the test's ability to connect to the correct port for the faucet service, as theServerPortfield is being used for this purpose.
- The
Portfield has been removed fromfaucetConfig.- A
ServerPortfield is present and used in the test to connect to the faucet service.Given these points, the review comment has been addressed in the code, and the test appears to be correctly configured to connect to the faucet service without the
Portfield.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the faucet service is correctly configured without the Port field. rg --type go 'faucetConfig' --context 5Length of output: 3453
testnet/launcher/docker.go (3)
13-14: The addition of import statements for
faucetandgatewaymodules is noted. Ensure that these modules are used appropriately in the subsequent code and that their integration follows best practices.200-200: The message indicating the readiness of the
gatewayis clear and reflects the correct port. This is good for user clarity.202-202: The final message indicates the successful launch of the network. This is a good practice to inform the user of the completion of the process.
testnet/launcher/docker.go
Outdated
| return fmt.Errorf("unable to wait for faucet to be ready - %w", err) | ||
| } | ||
|
|
||
| fmt.Println("Faucet ready to be accessed at http://127.0.0.1:99/ ...") |
There was a problem hiding this comment.
This port number should be read off the config or from constant probably, same with the gateway one below.
There was a problem hiding this comment.
The configs don't have external reading, but I agree this should be some form of constant. Changed 👍
| interval := time.Second | ||
|
|
||
| return retry.Do(func() error { | ||
| statusCode, _, err := fasthttp.Get(nil, fmt.Sprintf("http://127.0.0.1:%d/health/", n.cfg.faucetPort)) |
There was a problem hiding this comment.
Shouldn't we capture the body in case it has useful error information?
There was a problem hiding this comment.
No useful info from the health point responses for the faucet and the gw atm, but I guess we can do that in the future.
| interval := time.Second | ||
|
|
||
| return retry.Do(func() error { | ||
| statusCode, _, err := fasthttp.Get(nil, fmt.Sprintf("http://127.0.0.1:%d/health/", n.cfg.gatewayHTTPPort)) |
There was a problem hiding this comment.
Again with the body potentially to speed up debugging.
Why this change is needed
https://github.com/ten-protocol/ten-internal/issues/2701