#1 Create Nacos service discovery provider by MiaoShuYo · Pull Request #2 · ThreeMammals/Ocelot.Discovery.Nacos

MiaoShuYo · 2025-03-19T08:01:49Z

Closes #1

It is recommended to add support for Nacos #1

Related to

raman-m · 2025-03-19T11:10:12Z

🆗 Great! Thank you for creation this PR!
I'll review today...

raman-m

Please organize the folder structure more efficiently.
There is no need to create sub-sub folders.

Ocelot.Discovery.Nacos/Ocelot.Discovery.Nacos.sln

Ocelot.Discovery.Nacos/src/Ocelot.Discovery.Nacos/GlobalUsings.cs

Ocelot.Discovery.Nacos/test/Ocelot.Discovery.Nacos.UnitTest/MSTestSettings.cs

raman-m · 2025-03-19T15:30:38Z

The solution and folder structure now appear satisfactory to me.

raman-m

I've added GitHub Actions workflows. And the latest build job produced a number of warnings (see in Files changed).
Could you fix all of them please?

src/Nacos.cs

raman-m · 2025-03-19T15:57:07Z

src/Nacos.cs

+
+        return new Service(
+            id: instance.InstanceId,
+            hostAndPort: new ServiceHostAndPort(instance.Ip, instance.Port),


Suggested change

hostAndPort: new ServiceHostAndPort(instance.Ip, instance.Port),

hostAndPort: new(instance.Ip, instance.Port),

What is the scheme? Is the scheme known at this stage of building service?

When a client fetches a service, it will access the corresponding service based on the address information at the time of registration, so if the registration is for an HTTPS address, then the fetched service is HTTPS, and vice versa.

So, in the Nacos service discovery, scheme (protocol) management is not required, right?
For example, in Kubernetes we can manage the protocol while registering a service instance in the K8s registry.
In the next round of development, we will write acceptance tests that will show real usage in practice.

src/Nacos.cs

unit/Ocelot.Discovery.Nacos.UnitTests.csproj

src/Ocelot.Discovery.Nacos.csproj

raman-m

Please fix warnings generated by static code analysis tool during execution of latest GitHub action build.

src/NacosMiddlewareConfigurationProvider.cs

raman-m · 2025-03-21T08:23:28Z

Miao, what is the current code coverage achieved by unit tests?
Did you measure it?

MiaoShuYo · 2025-03-21T09:26:20Z

Miao, what is the current code coverage achieved by unit tests? Did you measure it?

raman-m · 2025-03-21T11:35:15Z

61% coverage is low.
We must achieve 100% because the lib has low amount of lines.

MiaoShuYo · 2025-03-21T15:07:51Z

61% coverage is low. We must achieve 100% because the lib has low amount of lines.

Test coverage 100%

raman-m

I recommend using IOcelotLogger instead of ILogger<Nacos>.

After this minor refactoring, I will verify the actual code coverage through unit tests once again.

src/Nacos.cs

raman-m

Thank you for addressing the issues from the previous code review! ❤️

The repository looks much better now with the initial solution and unit testing in place. However, I am concerned that it has not been tested with real HTTP requests. It would be prudent commence acceptance testing at this stage. We need to conduct load testing to ensure that the provider can handle a high number of parallel requests.

I'm going to add a commit with a draft version of the acceptance testing project.
Please find below my additional suggestions for improving the code.

raman-m · 2025-03-24T07:57:18Z

src/Nacos.cs

+            id: instance.InstanceId,
+            hostAndPort: new(instance.Ip, instance.Port),
+            name: instance.ServiceName,
+            version: metadata.GetValueOrDefault("version", "default"),


I cannot get this default value. Could you show me in Nacos docs please that default version is marked as "default"?

The version value is not explicitly defined as "default" in the Nacos documentation. The "default" value in the code is a custom fallback in case there is no "version" key in the metadata dictionary.

You confirmed that this is the issue. I am not certain but these values may be applicable: current, latest, default.
Question: How can the default be consumed by Ocelot or Nacos client?

The version can be customized through the configuration file.

raman-m · 2025-03-24T08:00:11Z

src/Nacos.cs

+    private static string FormatTag(KeyValuePair<string, string> kv)
+        => $"{WebUtility.UrlEncode(kv.Key)}={WebUtility.UrlEncode(kv.Value)}";
+
+    private static readonly string[] _reservedKeys = { "version", "group", "cluster", "namespace", "weight" };


It might be better to make this definition public.
Could you provide some links to metadata documentation that explain this?
I suggest writing an XML documentation (in the code) for the definition of the array.

In the nacos java version of the sdk, these are included by default, but in the nacos .net sdk these are not included by default and need to be added by yourself.

Please make this array publicly accessible, then.

src/NacosMiddlewareConfigurationProvider.cs

raman-m · 2025-03-24T08:39:57Z

src/OcelotBuilderExtensions.cs

+        builder.Services
+            .AddNacosAspNet(builder.Configuration,section)
+            .AddSingleton(NacosProviderFactory.Get)
+            .AddSingleton(NacosMiddlewareConfigurationProvider.Get);


Just a word of caution!
It might be premature to define this since you were inspired by the Consul provider and ConsulMiddlewareConfigurationProvider implementation. However, please note that this Consul config provider was developed specifically for the AddConfigStoredInConsul() feature, correct?
Therefore, let's keep it in the code for now, the next phase of acceptance testing will reveal the subsequent steps.

unit/LogEntry.cs

unit/TestLogger.cs

src/NacosMiddlewareConfigurationProvider.cs

raman-m · 2025-03-24T12:19:23Z

Hello, Miao!
As you've already noticed, I added the acceptance testing project. This is a very draft version, serving as a skeleton for a future well-developed project. The current code is compilable, with no errors after running dotnet build command.
The main testing class is NacosServiceDiscoveryTests ❕

Current pitfalls of the project:

The main class, NacosServiceDiscoveryTests, was copied from the KubernetesServiceDiscoveryTests class. As a result, the draft tests currently fail. We could draw inspiration from the ConsulServiceDiscoveryTests class as well.
The NacosServiceDiscoveryTests class inherits from ConcurrentSteps, which itself was copied from both ConcurrentSteps and Steps.
The remaining classes were copied from the Ocelot.AcceptanceTests codebase. This is far from ideal, and in the future, we must release a separate testing package. Don’t worry, I have a plan for that. For now, let’s save time by reusing code from the Ocelot repository.

Finally, I believe that releasing this package for .NET 6+8 with the draft acceptance testing project is acceptable. However, we must develop real tests, and they must pass.

Let's go!

raman-m · 2025-03-24T12:31:39Z

@MiaoShuYo commented on March 21

Test coverage 100%

🆗 I am curious to know which IDE and code coverage did you utilize for measurement?

MiaoShuYo · 2025-03-24T12:55:24Z

@MiaoShuYo commented on March 21

Test coverage 100%

🆗 I am curious to know which IDE and code coverage did you utilize for measurement?

Rider

raman-m · 2025-03-26T08:22:04Z

Please work on the acceptance tests as they are a required part of the PR process. Afterward, I will assist you in writing documentation. Do not hesitate to ask questions, I understand that acceptance testing can be a challenging milestone.

P.S. Currently I am occupied with the release of the Ocelot version for .NET9. Apologies for being very busy at moment.

MiaoShuYo · 2025-03-30T23:42:48Z

I am very sorry that I recently had an operation due to an accident, so I cannot carry out relevant development for the time being. It is expected to be suspended for three to five months.

raman-m · 2025-03-31T09:22:03Z

Wow, I'm saddened to hear that! I wish you a smooth and speedy recovery. 💟

P.S. Since development will be suspended for the next X months, I need to inform you that the package will not be released. The version for .NET 9 will be released after your return to development. In the meantime, within the next 1–2 months, I’ll try to release a very draft version for the current TFMs: net6.0, net7.0, and net8.0. However, I cannot guarantee that this draft version will function correctly in production. Regardless, I’ll wait for your return to development, as thorough testing of the future package in production will be highly desirable.

raman-m · 2025-05-27T14:51:36Z

@MiaoShuYo Hello, Miao!
How are you feeling these days?

FYI, I will review the pull request and merge it as-is because I need some actual code for CI-CD.
Following that I will set up the CI-CD workflows, and I will update the TFMs to net8.0 and net9.0.

… tests

raman-m

After updating the packages, I realized that updating the target framework monikers is impossible. So, TFMs remain net6.0 and net8.0 because the integrated nacos-sdk-csharp package explicitly targets these TFMs.

Ready for merging but not for delivery

Please note ❕ Following the merging of this PR we will proceed with following actions →

Configure CI-CD (GitHub Actions) for package delivery (assigned to @raman-m).
Develop acceptance tests, which are currently absent and are draft (assigned to @MiaoShuYo and @raman-m).

MiaoShuYo added 4 commits March 19, 2025 15:44

add nacose

470bee5

Block folder. idea

643c478

delete .idea

732fec4

Update .gitignore

7899622

raman-m assigned MiaoShuYo Mar 19, 2025

raman-m changed the title ~~ocelot.discovery.nacos~~ #1 Create Nacos service discovery provider Mar 19, 2025

raman-m requested changes Mar 19, 2025

View reviewed changes

MiaoShuYo and others added 5 commits March 19, 2025 22:54

Migrate file location, delete subfolders

589cf4d

Add blank lines

2100626

Migrate test project location.

ffbf1b5

Organize solution correctly

f39757d

Use file-scoped namespaces

30a0f34

CI/CD: add workflows aka GitHub Actions

0a3f426

raman-m requested changes Mar 19, 2025

View reviewed changes

MiaoShuYo added 3 commits March 20, 2025 10:52

Modified code

306196c

Update Ocelot.Discovery.Nacos.UnitTests.csproj

31308ca

Update pr.yml

1eed642

raman-m requested changes Mar 20, 2025

View reviewed changes

src/NacosMiddlewareConfigurationProvider.cs Outdated Show resolved Hide resolved

MiaoShuYo added 2 commits March 20, 2025 21:11

Fix warning

de2b498

Fix warning

ece0185

Fix VS IDE messages

71d81bc

Test coverage 100%

c7c0460

Review tests

d7c7c33

raman-m force-pushed the main_Ocelot.Discovery.Nacos branch from 4b53651 to d7c7c33 Compare March 22, 2025 09:38

raman-m requested changes Mar 22, 2025

View reviewed changes

src/Nacos.cs Outdated Show resolved Hide resolved

src/Nacos.cs Outdated Show resolved Hide resolved

raman-m and others added 2 commits March 22, 2025 13:20

No anonymous delegates: use named delegate

d962f9e

Replace the log component with IOcelotLoggerFactory

b82594c

raman-m requested changes Mar 24, 2025

View reviewed changes

MiaoShuYo and others added 3 commits March 24, 2025 17:09

Remove classes that are no longer on trial.

6767b6e

Add detailed notes to _reservedKeys

4547c55

Code review by @raman-m

bea32f4

raman-m reviewed Mar 24, 2025

View reviewed changes

src/NacosMiddlewareConfigurationProvider.cs Outdated Show resolved Hide resolved

raman-m added 2 commits March 24, 2025 14:43

Add acceptance testing project

7912f62

Update workflows to run acceptance tests

7ac2cae

Update src/NacosMiddlewareConfigurationProvider.cs

1a16f83

_reservedKeys changed to public

eec6bad

raman-m requested review from RaynaldM, ggnaegi and raman-m May 27, 2025 14:42

raman-m added 2 commits May 27, 2025 18:15

Temporarily disable acceptance testing due to the draft status of the…

950c5ae

… tests

Update package references

dd05752

raman-m approved these changes May 27, 2025

View reviewed changes

raman-m merged commit 3dd718f into main May 27, 2025
1 check passed

raman-m deleted the main_Ocelot.Discovery.Nacos branch May 27, 2025 16:33

raman-m mentioned this pull request May 27, 2025

Service discovery adds Nacos ThreeMammals/Ocelot#2268

Closed

	hostAndPort: new ServiceHostAndPort(instance.Ip, instance.Port),
	hostAndPort: new(instance.Ip, instance.Port),

Comments

Conversation

MiaoShuYo commented Mar 19, 2025 • edited by raman-m Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Closes #1

Related to

Uh oh!

raman-m commented Mar 19, 2025

Uh oh!

raman-m left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

raman-m commented Mar 19, 2025

Uh oh!

raman-m left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

raman-m left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

raman-m commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MiaoShuYo commented Mar 21, 2025

Uh oh!

raman-m commented Mar 21, 2025

Uh oh!

MiaoShuYo commented Mar 21, 2025

Uh oh!

raman-m left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

raman-m left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

raman-m commented Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

MiaoShuYo commented Mar 19, 2025 •

edited by raman-m

Loading

raman-m left a comment •

edited

Loading

raman-m commented Mar 21, 2025 •

edited

Loading

raman-m left a comment •

edited

Loading

raman-m left a comment •

edited

Loading

raman-m commented Mar 24, 2025 •

edited

Loading

raman-m commented Mar 26, 2025 •

edited

Loading