Read LAN Coordinates for a specific node #314

JocelynVelarde · 2024-04-11T00:30:01Z

Changes made

Added GET /v1/coordinate/node/+node into Coordinate.cs
Included new endpoints into ICoordinateEndpoint.cs
Added test CoordinateTest.cs

Issue ticket number and link

Refers to issue #313

marcin-krystianc

Tests are failing to compile (because of missing query options parameter).
Please note also that the 'Node' endpoint returns an array (https://developer.hashicorp.com/consul/api-docs/v1.9.x/coordinate#sample-response-2), instead of a single object.
I'm not sure how to handle it. It seems unlikely that there will be more than one entry in the result, but since the endpoint returns an array we probably should use an array in the result as well.

marcin-krystianc · 2024-04-11T07:28:31Z

Consul/Interfaces/ICoordinateEndpoint.cs

@@ -29,6 +29,7 @@ public interface ICoordinateEndpoint
    {
        Task<QueryResult<CoordinateDatacenterMap[]>> Datacenters(CancellationToken ct = default);
        Task<QueryResult<CoordinateEntry[]>> Nodes(CancellationToken ct = default);
+        Task<QueryResult<CoordinateEntry>> Node(string node, QueryOptions q, CancellationToken ct = default);


There are two overloads of Nodes endpoint (with and without QueryOptions q parameter).
We can Apply the same pattern for the new Node endpoint`

Sure noted! Already fixed 🚀

JocelynVelarde · 2024-04-11T14:57:05Z

Tests are failing to compile (because of missing query options parameter). Please note also that the 'Node' endpoint returns an array (https://developer.hashicorp.com/consul/api-docs/v1.9.x/coordinate#sample-response-2), instead of a single object. I'm not sure how to handle it. It seems unlikely that there will be more than one entry in the result, but since the endpoint returns an array we probably should use an array in the result as well.

I already fixed this on the recent commit, let me know if it needs further adjustments :)

marcin-krystianc · 2024-04-12T09:34:20Z

Tests are failing to compile (because of missing query options parameter). Please note also that the 'Node' endpoint returns an array (https://developer.hashicorp.com/consul/api-docs/v1.9.x/coordinate#sample-response-2), instead of a single object. I'm not sure how to handle it. It seems unlikely that there will be more than one entry in the result, but since the endpoint returns an array we probably should use an array in the result as well.

I already fixed this on the recent commit, let me know if it needs further adjustments :)

I've run the CI and there are some compilation issues. Please have a look.

marcin-krystianc · 2024-04-16T08:20:41Z

Hi @JocelynVelarde,

there is a problem with the coordinates endpoint, which returns empty responses for some time after starting the consul agent (e.g. https://github.com/G-Research/consuldotnet/actions/runs/8692709325/job/23862884528).
We can fix this by bumping the wait time and adding another check in the Base Fixture class:

JocelynVelarde · 2024-04-16T12:41:45Z

Hi @JocelynVelarde,

there is a problem with the coordinates endpoint, which returns empty responses for some time after starting the consul agent (e.g. https://github.com/G-Research/consuldotnet/actions/runs/8692709325/job/23862884528).

We can fix this by bumping the wait time and adding another check in the Base Fixture class:

Alright noted! Is there any way I can test the endpoint locally (making the request) so I can check the output before submitting for review?

marcin-krystianc · 2024-04-16T13:37:34Z

Alright noted! Is there any way I can test the endpoint locally (making the request) so I can check the output before submitting for review?

Sure thing, there is a description how to to start local agent for tests -> https://consuldot.net/docs/getting-started/setting-up-server

JocelynVelarde · 2024-04-16T13:39:35Z

Alright noted! Is there any way I can test the endpoint locally (making the request) so I can check the output before submitting for review?

Sure thing, there is a description how to to start local agent for tests -> https://consuldot.net/docs/getting-started/setting-up-server

Yeaaa! That's how I started running tests yesterday, so on the CLI I have to write the fetch request?

….com/JocelynVelarde/consuldotnet into G-Research#313-read-node-coordinates

marcin-krystianc · 2024-04-17T07:46:57Z

Consul/Coordinate.cs

+
+        public Task<QueryResult<CoordinateEntry[]>> Node(string node, CancellationToken ct = default)
+        {
+            return _client.Get<CoordinateEntry[]>(string.Format("/v1/coordinate/node/{0}", node)).Execute(ct);


Can be replaced with:

Suggested change

return _client.Get<CoordinateEntry[]>(string.Format("/v1/coordinate/node/{0}", node)).Execute(ct);

return Node(node, QueryOptions.Default, ct);

Alright done

marcin-krystianc · 2024-04-17T07:49:25Z

Consul.Test/BaseFixture.cs

@@ -67,6 +67,10 @@ public class BaseFixture : IAsyncLifetime
                    // Workaround for https://github.com/hashicorp/consul/issues/15061
                    await client.Agent.GetAgentMetrics();

+                    /*var nodesResponse = await client.Coordinate.Nodes();


Is it commented out by mistake?

Ohh let me activate it again

marcin-krystianc · 2024-04-17T07:53:06Z

Consul.Test/CoordinateTest.cs

+
+        [Fact]
+        public async Task Coordinate_GetNode()
+        {


Previous version of the test was better, we did actually use a real node name in the query before.

The thing is that with the node name I was receiving errors in the base fixture class

marcin-krystianc

Thanks, looks good. Just one final remark about removing unnecessary code.

marcin-krystianc · 2024-04-22T07:58:31Z

Consul.Test/CoordinateTest.cs

+
+            // Additional assertions can be added based on the expected properties of each node
+            // For example, if you expect certain properties like node name, ID, etc. to be present in each node detail
+            // Assert.NotNull(nodeDetails[0].PropertyName);
+            // Assert.Equal(expectedValue, nodeDetails[0].PropertyName);


Please remove that unnecessary lines:

Suggested change

// Additional assertions can be added based on the expected properties of each node

// For example, if you expect certain properties like node name, ID, etc. to be present in each node detail

// Assert.NotNull(nodeDetails[0].PropertyName);

// Assert.Equal(expectedValue, nodeDetails[0].PropertyName);

….com/JocelynVelarde/consuldotnet into G-Research#313-read-node-coordinates

marcin-krystianc

Now it is perfect, thank you!
I need to sort out macOS CI failures before merging it.

Read LAN Coordinates for a specific node

a815eb7

marcin-krystianc reviewed Apr 11, 2024

View reviewed changes

JocelynVelarde added 2 commits April 11, 2024 08:19

Fix endpoint overload

ec27430

Added query parameter to test and changed to return node array

47f160a

Test fix

ce83113

JocelynVelarde and others added 2 commits April 15, 2024 10:29

fixed unit testing

81f30a8

style: fix whitespaces

0d9f8bc

JocelynVelarde and others added 8 commits April 16, 2024 09:28

updated wait time and condition in base fixture class

8155cdd

Merge branch 'G-Research#313-read-node-coordinates' of https://github…

853e938

….com/JocelynVelarde/consuldotnet into G-Research#313-read-node-coordinates

style: fix whitespaces

79bac98

syntax error fix

7be9a9c

.

d06c16a

style: fix whitespaces

f3d89c7

test alternative for node coordinate

ed2bd2c

fix merge

2f2941a

marcin-krystianc reviewed Apr 17, 2024

View reviewed changes

JocelynVelarde and others added 2 commits April 21, 2024 23:15

reverted test for individual node

5b2ed3e

style: fix whitespaces

1e89e43

marcin-krystianc reviewed Apr 22, 2024

View reviewed changes

JocelynVelarde added 2 commits April 22, 2024 08:12

deleted comments

f147e92

Merge branch 'G-Research#313-read-node-coordinates' of https://github…

ef9e81d

….com/JocelynVelarde/consuldotnet into G-Research#313-read-node-coordinates

marcin-krystianc approved these changes Apr 23, 2024

View reviewed changes

marcin-krystianc added 2 commits April 25, 2024 13:25

Merge branch 'master' into G-Research#313-read-node-coordinates

c1c1840

Merge branch 'master' into G-Research#313-read-node-coordinates

f462ed0

marcin-krystianc merged commit ed3518c into G-Research:master Apr 29, 2024
227 checks passed

This was referenced May 7, 2024

Update 2-supported-apis.mdx #320

Merged

FEATURE REQUEST: Read LAN Coordinates for a node #313

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read LAN Coordinates for a specific node #314

Read LAN Coordinates for a specific node #314

JocelynVelarde commented Apr 11, 2024

marcin-krystianc left a comment

marcin-krystianc Apr 11, 2024

JocelynVelarde Apr 11, 2024

JocelynVelarde commented Apr 11, 2024

marcin-krystianc commented Apr 12, 2024

marcin-krystianc commented Apr 16, 2024

JocelynVelarde commented Apr 16, 2024

marcin-krystianc commented Apr 16, 2024

JocelynVelarde commented Apr 16, 2024

marcin-krystianc Apr 17, 2024

JocelynVelarde Apr 19, 2024

marcin-krystianc Apr 17, 2024

JocelynVelarde Apr 19, 2024

marcin-krystianc Apr 17, 2024

JocelynVelarde Apr 19, 2024

marcin-krystianc left a comment

marcin-krystianc Apr 22, 2024

marcin-krystianc left a comment

	return _client.Get<CoordinateEntry[]>(string.Format("/v1/coordinate/node/{0}", node)).Execute(ct);
	return Node(node, QueryOptions.Default, ct);

Read LAN Coordinates for a specific node #314

Read LAN Coordinates for a specific node #314

Conversation

JocelynVelarde commented Apr 11, 2024

Changes made

Issue ticket number and link

marcin-krystianc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JocelynVelarde commented Apr 11, 2024

marcin-krystianc commented Apr 12, 2024

marcin-krystianc commented Apr 16, 2024

JocelynVelarde commented Apr 16, 2024

marcin-krystianc commented Apr 16, 2024

JocelynVelarde commented Apr 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcin-krystianc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcin-krystianc left a comment

Choose a reason for hiding this comment