Added list nodes for connect-capable service with endpoints #312

JocelynVelarde · 2024-04-05T16:38:33Z

Changes made

Added GET /health/connect/:service into Health.cs
Included new endpoints into IHealthEndpoint.cs

Issue ticket number and link

Refers to issue #311

Next tasks

Write corresponding tests

JocelynVelarde · 2024-04-05T16:41:47Z

Hello! I would appreciate some feedback/review regarding this new endpoint support. 🐝

marcin-krystianc

Hi @JocelynVelarde,

Thank you for raising this PR. I've left a comment regarding the importance of maintaining backward compatibility and avoiding unnecessary logic in our library. Apart from that, could you please implement tests for the new API that is being introduced?

marcin-krystianc · 2024-04-08T11:19:50Z

Consul/Health.cs

+            switch (healthType)
+            {
+                case "connectHealth":
+                    path = "/v1/health/connect/{0}";
+                    break;
+                default:
+                    path = "/v1/health/service/{0}";
+                    break;
+            }
+            var req = _client.Get<ServiceEntry[]>(string.Format(path, service), q, filter);


I know that the documentation mentions that the connect and service endpoints are nearly identical, but I don't think it's a good idea to rely on that. I would rather split this into two separate implementations, one for connect and one for service endpoints.

Okei noted, so instead of implementing the switch case, I should implement as a separate entity?

Okei noted, so instead of implementing the switch case, I should implement as a separate entity?

yes, I think it is going to be better that way.

marcin-krystianc · 2024-04-08T11:22:40Z

Consul/Interfaces/IHealthEndpoint.cs

@@ -36,7 +36,9 @@ public interface IHealthEndpoint
        Task<QueryResult<ServiceEntry[]>> Service(string service, string tag, CancellationToken ct = default);
        Task<QueryResult<ServiceEntry[]>> Service(string service, string tag, bool passingOnly, CancellationToken ct = default);
        Task<QueryResult<ServiceEntry[]>> Service(string service, string tag, bool passingOnly, QueryOptions q, CancellationToken ct = default);
-        Task<QueryResult<ServiceEntry[]>> Service(string service, string tag, bool passingOnly, QueryOptions q, Filter filter, CancellationToken ct = default);
+        Task<QueryResult<ServiceEntry[]>> Service(string service, string tag, bool passingOnly, QueryOptions q, Filter filter, string healthType, CancellationToken ct = default);


It is not ok, to change the public method signature in a way that makes it incompatible with previous versions.
If a new parameter is needed, then the new method overload needs to be added (without modifying the existing one).

Alright so, leave the method signature as it was and just add a new one with the new parameter.

JocelynVelarde · 2024-04-26T05:35:07Z

Hello! I was getting some errors with the BaseFixture.cs class, I was wondering if someone could explain why this is happening, greetings.

marcin-krystianc

Hello! I was getting some errors with the BaseFixture.cs class, I was wondering if someone could explain why this is happening, greetings.

Hi, I'm not sure which errors you mean exactly. I see that everything fails because the response is empty. I suggest have a look at Agent_Service_Register_With_Connect and see how to register service with a "connect" information.

JocelynVelarde · 2024-05-01T05:29:03Z

Hello! I was getting some errors with the BaseFixture.cs class, I was wondering if someone could explain why this is happening, greetings.

Hi, I'm not sure which errors you mean exactly. I see that everything fails because the response is empty. I suggest have a look at Agent_Service_Register_With_Connect and see how to register service with a "connect" information.

From what I can see, Connect is equivalent to Service, except that Service will return the ones that are Connect-enabled. The new endpoint for Connect in Health.cs is correct? I just noticed that there is a Filter parameter that I was not using in the test request. Here
var checks = await _client.Health.Connect(svcID, "", false, QueryOptions.Default);
I'm not sure if this will fix the error.

marcin-krystianc · 2024-05-02T10:01:28Z

Hello! I was getting some errors with the BaseFixture.cs class, I was wondering if someone could explain why this is happening, greetings.

Hi, I'm not sure which errors you mean exactly. I see that everything fails because the response is empty. I suggest have a look at Agent_Service_Register_With_Connect and see how to register service with a "connect" information.

From what I can see, Connect is equivalent to Service, except that Service will return the ones that are Connect-enabled. The new endpoint for Connect in Health.cs is correct? I just noticed that there is a Filter parameter that I was not using in the test request. Here var checks = await _client.Health.Connect(svcID, "", false, QueryOptions.Default); I'm not sure if this will fix the error.

I think, the key to properly test it is to create conenct-enabled service, e.g.:

            var destinationServiceRegistrationParameters = new AgentServiceRegistration
            {
                ID = destinationServiceID,
                Name = destinationServiceID,
                Port = 8000,
                Check = new AgentServiceCheck
                {
                    TTL = TimeSpan.FromSeconds(15)
                },
                Connect = new AgentServiceConnect
                {
                    SidecarService = new AgentServiceRegistration
                    {
                        Port = 8001
                    }
                }
            };

marcin-krystianc

Hi @JocelynVelarde,

the changes look good now. and the tests are successful in the CI too. There is a small issue with GH reporting status to the RP though. Can you push another commit or two (e.g. add an empty line and remove it) maybe that will resolve the issue with tests statuses?

marcin-krystianc · 2024-05-07T09:29:37Z

Hi @JocelynVelarde,

the changes look good now. and the tests are successful in the CI too. There is a small issue with GH reporting status to the RP though. Can you push another commit or two (e.g. add an empty line and remove it) maybe that will resolve the issue with tests statuses?

Nevermind, I closed and reopened the PR to trigger the workflow on the latest commit.

marcin-krystianc · 2024-05-07T09:48:05Z

Consul.Test/HealthTest.cs

+
+
+
+


nit: ```suggestion

I fixed the whitespaces on the last commit.

…ub.com/JocelynVelarde/consuldotnet into G-Research#311-connect-capable-service

marcin-krystianc · 2024-05-07T20:20:53Z

I think that the Catalog_GetNodesForMeshCapableService got flaky after adding the `Health_Connect' test.
We need to keep eye on it, if it is going to fail frequently, then we will need to investigate the root cause.

Added list nodes for connect-capable service, included new endpoints

34651db

JocelynVelarde changed the title ~~Added list nodes for connect-capable service, included new endpoints~~ Added list nodes for connect-capable service with endpoints Apr 5, 2024

marcin-krystianc reviewed Apr 8, 2024

View reviewed changes

JocelynVelarde and others added 2 commits April 25, 2024 23:31

separated connect endpoint from service, created potential test

de1cea9

style: fix whitespaces

30e04c6

Merge branch 'master' into G-Research#311-connect-capable-service

46f05b4

marcin-krystianc approved these changes Apr 29, 2024

View reviewed changes

Merge branch 'master' into G-Research#311-connect-capable-service

8f214c8

marcin-krystianc requested changes Apr 29, 2024

View reviewed changes

JocelynVelarde and others added 2 commits May 6, 2024 07:54

wrote corresponding tests for connecting capable service

e6b2cd7

style: fix whitespaces

6ab104d

marcin-krystianc previously approved these changes May 7, 2024

View reviewed changes

marcin-krystianc closed this May 7, 2024

marcin-krystianc reopened this May 7, 2024

marcin-krystianc reviewed May 7, 2024

View reviewed changes

JocelynVelarde added 2 commits May 7, 2024 11:33

fixed whitespace

aadb210

Merge branch 'G-Research#311-connect-capable-service' of https://gith…

46da22e

…ub.com/JocelynVelarde/consuldotnet into G-Research#311-connect-capable-service

JocelynVelarde dismissed marcin-krystianc’s stale review via 46da22e May 7, 2024 17:33

marcin-krystianc approved these changes May 7, 2024

View reviewed changes

marcin-krystianc merged commit 80c0fa8 into G-Research:master May 7, 2024
227 checks passed

This was referenced May 7, 2024

Update 2-supported-apis.mdx #320

Merged

FEATURE REQUEST: Support for List Nodes for Connect-capable Service #311

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added list nodes for connect-capable service with endpoints #312

Added list nodes for connect-capable service with endpoints #312

JocelynVelarde commented Apr 5, 2024 •

edited

Loading

JocelynVelarde commented Apr 5, 2024

marcin-krystianc left a comment

marcin-krystianc Apr 8, 2024

JocelynVelarde Apr 15, 2024

marcin-krystianc Apr 16, 2024

marcin-krystianc Apr 8, 2024

JocelynVelarde Apr 15, 2024 •

edited

Loading

JocelynVelarde commented Apr 26, 2024

marcin-krystianc left a comment

JocelynVelarde commented May 1, 2024

marcin-krystianc commented May 2, 2024

marcin-krystianc left a comment

marcin-krystianc commented May 7, 2024

marcin-krystianc May 7, 2024

JocelynVelarde May 7, 2024

marcin-krystianc commented May 7, 2024

Added list nodes for connect-capable service with endpoints #312

Added list nodes for connect-capable service with endpoints #312

Conversation

JocelynVelarde commented Apr 5, 2024 • edited Loading

Changes made

Issue ticket number and link

Next tasks

JocelynVelarde commented Apr 5, 2024

marcin-krystianc left a comment

Choose a reason for hiding this comment

marcin-krystianc Apr 8, 2024

Choose a reason for hiding this comment

JocelynVelarde Apr 15, 2024

Choose a reason for hiding this comment

marcin-krystianc Apr 16, 2024

Choose a reason for hiding this comment

marcin-krystianc Apr 8, 2024

Choose a reason for hiding this comment

JocelynVelarde Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

JocelynVelarde commented Apr 26, 2024

marcin-krystianc left a comment

Choose a reason for hiding this comment

JocelynVelarde commented May 1, 2024

marcin-krystianc commented May 2, 2024

marcin-krystianc left a comment

Choose a reason for hiding this comment

marcin-krystianc commented May 7, 2024

marcin-krystianc May 7, 2024

Choose a reason for hiding this comment

JocelynVelarde May 7, 2024

Choose a reason for hiding this comment

marcin-krystianc commented May 7, 2024

JocelynVelarde commented Apr 5, 2024 •

edited

Loading

JocelynVelarde Apr 15, 2024 •

edited

Loading