-
Notifications
You must be signed in to change notification settings - Fork 131
feat: Add print for network develop #3042
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Hi @gfalaschi. Thanks for your PR! I am @adamjensenbot.
Make sure this PR appears in the liqo changelog, adding one of the following labels:
|
|
Hi @gfalaschi, thanks for your PR. First of all, should you enrich the error returned by the functions instead of just printing it? (e.g fmt.Errorf("my error message: %w", err)) |
| // Wait for Connections on both cluster to be created. | ||
| conn2, err := cluster2.waiter.ForConnection(ctx, gwServer.Namespace, cluster1.localClusterID) | ||
| if err != nil { | ||
| fmt.Print("GeneveTunnel doasn't exists in provider") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Connection resource is not related to the creation of the Geneve tunnel and the internal network. The Connection is related to the external network tunnel (e.g wireguard).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's where the Geneve tunnel inside the gateway is created.
| } | ||
| conn1, err := cluster1.waiter.ForConnection(ctx, gwClient.Namespace, cluster2.localClusterID) | ||
| if err != nil { | ||
| fmt.Print("GeneveTunnel doasn't exists in consumer") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
| return err | ||
| } | ||
| if err := cluster2.waiter.ForConnectionEstablished(ctx, conn2); err != nil { | ||
| fmt.Printf("Connection failed: port %v on cluster %v is closed or unreachable (IP: %v)\n", gwClient.Spec.Endpoint.Port, gwClient.Name, gwClient.Status.InternalEndpoint.IP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you should print the spec.endpoint.addresses instead of the status.internalEndpoint.IP
|
|
||
| // Wait for Connections on both cluster cluster to be established | ||
| if err := cluster1.waiter.ForConnectionEstablished(ctx, conn1); err != nil { | ||
| fmt.Printf("Connection failed: port %v on cluster %v is closed or unreachable (IP: %v)\n", gwServer.Spec.Endpoint.Port, gwServer.Name, gwClient.Status.InternalEndpoint.IP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here print status.endpoint.addresses instead of status.internalEndpoint.IP
|
|
||
| // Wait for Connections on both cluster cluster to be established | ||
| if err := cluster1.waiter.ForConnectionEstablished(ctx, conn1); err != nil { | ||
| fmt.Printf("Connection failed: port %v on cluster %v is closed or unreachable (IP: %v)\n", gwServer.Spec.Endpoint.Port, gwServer.Name, gwClient.Status.InternalEndpoint.IP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message is wrong, the unreachable port is the one on the server cluster, not on the client one
bec7493 to
2f76836
Compare
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes #(issue)
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Please also note any relevant details for your test configuration.