Re: [PR] feat(nacos-sd): add auth and filtering parameters for Nacos service d… [hertzbeat]

2026-04-17 Thread via GitHub 


ZackFair5185034 commented on PR #4099:
URL: https://github.com/apache/hertzbeat/pull/4099#issuecomment-4266899438

   This is a great suggestion and very valuable


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] feat(nacos-sd): add auth and filtering parameters for Nacos service d… [hertzbeat]

2026-03-29 Thread via GitHub 


zqr10159 merged PR #4099:
URL: https://github.com/apache/hertzbeat/pull/4099


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] feat(nacos-sd): add auth and filtering parameters for Nacos service d… [hertzbeat]

2026-03-29 Thread via GitHub 


Copilot commented on code in PR #4099:
URL: https://github.com/apache/hertzbeat/pull/4099#discussion_r3005880081


##
hertzbeat-collector/hertzbeat-collector-basic/src/main/java/org/apache/hertzbeat/collector/collect/registry/discovery/impl/NacosDiscoveryClient.java:
##
@@ -97,8 +119,25 @@ public List getServices() {
 }
 List serviceInstanceList = Lists.newArrayList();
 try {
-for (String serviceName : namingService.getServicesOfServer(0, 
).getData()) {
-namingService.getAllInstances(serviceName).forEach(instance ->
+List services ;
+if(StringUtils.hasText(localConnectConfig.getGroupName())) {
+  services = namingService.getServicesOfServer(0, , 
localConnectConfig.getGroupName()).getData();
+} else {
+  services = namingService.getServicesOfServer(0, ).getData();
+}
+
+for (String serviceName : services) {
+
if(StringUtils.hasText(localConnectConfig.getServiceName())&&!serviceName.equals(localConnectConfig.getServiceName())){
+continue;
+}
+List instances;
+if(StringUtils.hasText(localConnectConfig.getGroupName())){
+instances = namingService.getAllInstances(serviceName, 
localConnectConfig.getGroupName());
+}else{
+instances = namingService.getAllInstances(serviceName);
+}

Review Comment:
   New groupName/serviceName branches are not covered by unit tests (e.g., 
using getServicesOfServer(page,size,groupName), 
getAllInstances(service,groupName), and verifying serviceName filtering). Add 
test cases for: (1) groupName set, (2) serviceName set, and (3) both set, 
asserting the correct NamingService methods are invoked and results are 
filtered as expected.



##
hertzbeat-manager/src/main/resources/define/app-nacos_sd.yml:
##
@@ -42,6 +42,78 @@ params:
   ja-JP: ポート
 type: number
 required: true
+  - field: __nacos_sd_username__
+name:
+  zh-CN: Nacos 服务发现 Username
+  en-US: Nacos Service Discovery User Name
+  ja-JP: ユーザー名
+type: text
+required: false
+  - field: __nacos_sd_password__
+name:
+  zh-CN: Nacos 服务发现 Password
+  en-US: Nacos Service Discovery Password
+  ja-JP: パスワード
+type: password
+required: false
+  - field: __nacos_sd_namespace__
+name:
+  zh-CN: Nacos 服务发现 Namespace
+  en-US: Nacos Service Discovery Namespace
+  ja-JP: 名前空間
+type: text
+required: false
+  - field: __nacos_sd_groupName__
+name:
+  zh-CN: Nacos 服务发现 Group Name
+  en-US: Nacos Service Discovery Group Name
+  ja-JP: グループ名
+type: text
+required: false
+  - field: __nacos_sd_serviceName__
+name:
+  zh-CN: Nacos 服务发现 Service Name
+  en-US: Nacos Service Discovery Service Name
+  ja-JP: サービス名
+type: text
+required: false
+
+  - field: __nacos_sd_username__
+name:
+  zh-CN: Nacos 服务发现 Username
+  en-US: Nacos Service Discovery User Name
+  ja-JP: ユーザー名
+type: text
+required: false
+  - field: __nacos_sd_password__
+name:
+  zh-CN: Nacos 服务发现 Password
+  en-US: Nacos Service Discovery Password
+  ja-JP: パスワード
+type: password
+required: false
+  - field: __nacos_sd_namespace__
+name:
+  zh-CN: Nacos 服务发现 Namespace
+  en-US: Nacos Service Discovery Namespace
+  ja-JP: 名前空間
+type: text
+required: false
+  - field: __nacos_sd_groupName__
+name:
+  zh-CN: Nacos 服务发现 Group Name
+  en-US: Nacos Service Discovery Group Name
+  ja-JP: グループ名
+type: text
+required: false
+  - field: __nacos_sd_serviceName__
+name:
+  zh-CN: Nacos 服务发现 Service Name
+  en-US: Nacos Service Discovery Service Name
+  ja-JP: サービス名
+type: text
+required: false
+
 

Review Comment:
   The params section defines the same five fields twice 
(__nacos_sd_username__/password/namespace/groupName/serviceName). This will 
likely render duplicate inputs and can cause ambiguous parameter resolution. 
Remove the duplicated second block so each field is defined only once.
   ```suggestion
   
   ```



##
hertzbeat-collector/hertzbeat-collector-basic/src/main/java/org/apache/hertzbeat/collector/collect/registry/discovery/impl/NacosDiscoveryClient.java:
##
@@ -97,8 +119,25 @@ public List getServices() {
 }
 List serviceInstanceList = Lists.newArrayList();
 try {
-for (String serviceName : namingService.getServicesOfServer(0, 
).getData()) {
-namingService.getAllInstances(serviceName).forEach(instance ->
+List services ;
+if(StringUtils.hasText(localConnectConfig.getGroupName())) {
+  services = namingService.getServicesOfServer(0, , 
localConnectConfig.getGroupName()).getData();
+

Re: [PR] feat(nacos-sd): add auth and filtering parameters for Nacos service d… [hertzbeat]

2026-03-28 Thread via GitHub 


pentium100 commented on PR #4099:
URL: https://github.com/apache/hertzbeat/pull/4099#issuecomment-4149420467

   ok, i will modify this PR, and push it again.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] feat(nacos-sd): add auth and filtering parameters for Nacos service d… [hertzbeat]

2026-03-28 Thread via GitHub 


Copilot commented on code in PR #4099:
URL: https://github.com/apache/hertzbeat/pull/4099#discussion_r3005652718


##
hertzbeat-collector/hertzbeat-collector-basic/src/main/java/org/apache/hertzbeat/collector/collect/registry/discovery/impl/NacosDiscoveryClient.java:
##
@@ -97,7 +111,17 @@ public List getServices() {
 }
 List serviceInstanceList = Lists.newArrayList();
 try {
-for (String serviceName : namingService.getServicesOfServer(0, 
).getData()) {
+List services ;
+if(StringUtils.hasText(localConnectConfig.getGroupName())) {
+  services = namingService.getServicesOfServer(0, , 
localConnectConfig.getGroupName()).getData();
+} else {
+  services = namingService.getServicesOfServer(0, ).getData();
+}
+
+for (String serviceName : services) {
+
if(StringUtils.hasText(localConnectConfig.getServiceName())&&!serviceName.equals(localConnectConfig.getServiceName())){
+continue;
+}
 namingService.getAllInstances(serviceName).forEach(instance ->

Review Comment:
   When `groupName` is specified, service names are fetched from that group, 
but instance lookup still uses `namingService.getAllInstances(serviceName)` 
(default group). This will return instances from the wrong group (or none). 
Pass `localConnectConfig.getGroupName()` to the instance query as well (and 
ensure the default-group behavior remains unchanged when `groupName` is blank).
   ```suggestion
   List instances;
   if (StringUtils.hasText(localConnectConfig.getGroupName())) {
   instances = namingService.getAllInstances(serviceName, 
localConnectConfig.getGroupName());
   } else {
   instances = namingService.getAllInstances(serviceName);
   }
   instances.forEach(instance ->
   ```



##
hertzbeat-collector/hertzbeat-collector-basic/src/main/java/org/apache/hertzbeat/collector/collect/registry/discovery/impl/NacosDiscoveryClient.java:
##
@@ -55,7 +63,13 @@ public void initClient(ConnectConfig connectConfig) {
 try {
 
 localConnectConfig = connectConfig;
-namingService = 
NamingFactory.createNamingService(connectConfig.getHost() + ":" + 
connectConfig.getPort());
+Properties properties = new Properties();
+properties.put("serverAddr", connectConfig.getHost() + ":" + 
connectConfig.getPort());
+properties.put("username", connectConfig.getUsername());
+properties.put("password", connectConfig.getPassword());
+properties.put("namespace", connectConfig.getNamespace());

Review Comment:
   `Properties` (which extends `Hashtable`) does not allow null values. Since 
`username`/`password`/`namespace` are optional, `properties.put("username", 
connectConfig.getUsername())` etc. will throw `NullPointerException` when any 
of these are unset. Only set these properties when they are non-empty (e.g., 
guard with `StringUtils.hasText(...)`) or use a helper that skips null/blank 
values.
   ```suggestion
   if (StringUtils.hasText(connectConfig.getUsername())) {
   properties.put("username", connectConfig.getUsername());
   }
   if (StringUtils.hasText(connectConfig.getPassword())) {
   properties.put("password", connectConfig.getPassword());
   }
   if (StringUtils.hasText(connectConfig.getNamespace())) {
   properties.put("namespace", connectConfig.getNamespace());
   }
   ```



##
hertzbeat-collector/hertzbeat-collector-basic/src/main/java/org/apache/hertzbeat/collector/collect/registry/discovery/impl/NacosDiscoveryClient.java:
##
@@ -55,7 +63,13 @@ public void initClient(ConnectConfig connectConfig) {
 try {
 
 localConnectConfig = connectConfig;
-namingService = 
NamingFactory.createNamingService(connectConfig.getHost() + ":" + 
connectConfig.getPort());
+Properties properties = new Properties();
+properties.put("serverAddr", connectConfig.getHost() + ":" + 
connectConfig.getPort());
+properties.put("username", connectConfig.getUsername());
+properties.put("password", connectConfig.getPassword());
+properties.put("namespace", connectConfig.getNamespace());
+
+namingService = NamingFactory.createNamingService(properties);
 

Review Comment:
   `initClient` now calls `NamingFactory.createNamingService(Properties)`, but 
the existing unit test `NacosDiscoveryClientTest#testInitClientSuccess` 
mocks/verify the `createNamingService(String)` overload. This change will break 
that test (and it currently doesn't assert that username/password/namespace are 
passed). Update the tests to m