Draft: <feature>[core]: Add external service configuration APIs#4186
Closed
MatheMatrix wants to merge 1 commit into
Closed
Draft: <feature>[core]: Add external service configuration APIs#4186MatheMatrix wants to merge 1 commit into
MatheMatrix wants to merge 1 commit into
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (47)
Walkthrough本PR新增外部服务配置功能的完整实现链路，包括数据库表定义、API契约、管理器CRUD与全节点编排、SDK生成对象与测试框架支持，使系统能够对外部监控等服务进行配置的增删改查与集群范围内的配置下发。 Changes外部服务配置管理功能
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Collaborator
|
Comment from yaohua.wu: Review: MR !10144 — ZSV-12395Background
关联 MR
🟡 Warning
🟢 Suggestion
Coverage
Verdict: REVISION_REQUIRED无 🔴 Critical。建议在去 Draft 前优先处理 Warning #1（写库/下发事务一致性 + serviceType 校验） 与 #2（createDate 🤖 Robot Reviewer |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@header/src/main/java/org/zstack/header/core/external/service/APIUpdateExternalServiceConfigurationMsgDoc_zh_cn.groovy`:
- Around line 33-41: The documentation incorrectly exposes the "configuration"
field as an updatable parameter in
APIUpdateExternalServiceConfigurationMsgDoc_zh_cn.groovy; according to
ExternalServiceManagerImpl.updateExternalServiceConfiguration() the API only
updates "description" and does not apply configuration changes or call
applyExternalServiceConfigurationToAllNodes(), so remove the "configuration"
column entry (or update its desc to state it is ignored/unmodifiable and that
configuration changes require delete+create and will change the UUID), leaving
only "description" as the modifiable field.
In
`@sdk/src/main/java/org/zstack/sdk/QueryExternalServiceConfigurationResult.java`:
- Around line 6-12: The field inventories and its accessor methods in
QueryExternalServiceConfigurationResult use a raw java.util.List; change them to
a parameterized list to restore type safety by declaring the field as
java.util.List<ExternalServiceConfigurationInventory> (or the correct inventory
type used in the SDK), update the signatures of
setInventories(java.util.List<ExternalServiceConfigurationInventory>
inventories) and getInventories() to return
java.util.List<ExternalServiceConfigurationInventory>, and add the matching
import if needed so callers get compile-time checks and proper IDE completion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ef85bf51-81e2-42f6-afae-a79c0c67a367
⛔ Files ignored due to path filters (2)
conf/persistence.xmlis excluded by!**/*.xmlconf/serviceConfig/externalService.xmlis excluded by!**/*.xml
📒 Files selected for processing (39)
conf/db/zsv/V5.1.0__schema.sqlcore/src/main/java/org/zstack/core/externalservice/ExternalService.javacore/src/main/java/org/zstack/core/externalservice/ExternalServiceManagerImpl.javaheader/src/main/java/org/zstack/header/core/external/service/APIAddExternalServiceConfigurationEvent.javaheader/src/main/java/org/zstack/header/core/external/service/APIAddExternalServiceConfigurationEventDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/core/external/service/APIAddExternalServiceConfigurationMsg.javaheader/src/main/java/org/zstack/header/core/external/service/APIAddExternalServiceConfigurationMsgDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/core/external/service/APIDeleteExternalServiceConfigurationEvent.javaheader/src/main/java/org/zstack/header/core/external/service/APIDeleteExternalServiceConfigurationEventDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/core/external/service/APIDeleteExternalServiceConfigurationMsg.javaheader/src/main/java/org/zstack/header/core/external/service/APIDeleteExternalServiceConfigurationMsgDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/core/external/service/APIQueryExternalServiceConfigurationMsg.javaheader/src/main/java/org/zstack/header/core/external/service/APIQueryExternalServiceConfigurationMsgDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/core/external/service/APIQueryExternalServiceConfigurationReply.javaheader/src/main/java/org/zstack/header/core/external/service/APIQueryExternalServiceConfigurationReplyDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/core/external/service/APIUpdateExternalServiceConfigurationEvent.javaheader/src/main/java/org/zstack/header/core/external/service/APIUpdateExternalServiceConfigurationEventDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/core/external/service/APIUpdateExternalServiceConfigurationMsg.javaheader/src/main/java/org/zstack/header/core/external/service/APIUpdateExternalServiceConfigurationMsgDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/core/external/service/ApplyExternalConfigurationResult.javaheader/src/main/java/org/zstack/header/core/external/service/ApplyExternalServiceConfigurationMsg.javaheader/src/main/java/org/zstack/header/core/external/service/ApplyExternalServiceConfigurationReply.javaheader/src/main/java/org/zstack/header/core/external/service/ExternalServiceConfigurationInventory.javaheader/src/main/java/org/zstack/header/core/external/service/ExternalServiceConfigurationInventoryDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/core/external/service/ExternalServiceConfigurationVO.javaheader/src/main/java/org/zstack/header/core/external/service/ExternalServiceConfigurationVO_.javaheader/src/main/java/org/zstack/header/core/external/service/ExternalServiceInventory.javasdk/src/main/java/org/zstack/sdk/AddExternalServiceConfigurationAction.javasdk/src/main/java/org/zstack/sdk/AddExternalServiceConfigurationResult.javasdk/src/main/java/org/zstack/sdk/DeleteExternalServiceConfigurationAction.javasdk/src/main/java/org/zstack/sdk/DeleteExternalServiceConfigurationResult.javasdk/src/main/java/org/zstack/sdk/ExternalServiceConfigurationInventory.javasdk/src/main/java/org/zstack/sdk/ExternalServiceInventory.javasdk/src/main/java/org/zstack/sdk/QueryExternalServiceConfigurationAction.javasdk/src/main/java/org/zstack/sdk/QueryExternalServiceConfigurationResult.javasdk/src/main/java/org/zstack/sdk/SourceClassMap.javasdk/src/main/java/org/zstack/sdk/UpdateExternalServiceConfigurationAction.javasdk/src/main/java/org/zstack/sdk/UpdateExternalServiceConfigurationResult.javatestlib/src/main/java/org/zstack/testlib/ApiHelper.groovy
Comment on lines
+6
to
+12
| public java.util.List inventories; | ||
| public void setInventories(java.util.List inventories) { | ||
| this.inventories = inventories; | ||
| } | ||
| public java.util.List getInventories() { | ||
| return this.inventories; | ||
| } |
There was a problem hiding this comment.
使用原始类型导致类型安全缺失
inventories 字段声明为原始类型 java.util.List，未指定泛型参数。这会导致：
- 调用方无法在编译期获得类型检查，可能引发
ClassCastException - IDE 无法提供准确的代码补全与类型推断
- 不符合 Java 泛型最佳实践
🔧 修复建议
- public java.util.List inventories;
- public void setInventories(java.util.List inventories) {
+ public java.util.List<ExternalServiceConfigurationInventory> inventories;
+ public void setInventories(java.util.List<ExternalServiceConfigurationInventory> inventories) {
this.inventories = inventories;
}
- public java.util.List getInventories() {
+ public java.util.List<ExternalServiceConfigurationInventory> getInventories() {
return this.inventories;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@sdk/src/main/java/org/zstack/sdk/QueryExternalServiceConfigurationResult.java`
around lines 6 - 12, The field inventories and its accessor methods in
QueryExternalServiceConfigurationResult use a raw java.util.List; change them to
a parameterized list to restore type safety by declaring the field as
java.util.List<ExternalServiceConfigurationInventory> (or the correct inventory
type used in the SDK), update the signatures of
setInventories(java.util.List<ExternalServiceConfigurationInventory>
inventories) and getInventories() to return
java.util.List<ExternalServiceConfigurationInventory>, and add the matching
import if needed so callers get compile-time checks and proper IDE completion.
MatheMatrix
force-pushed
the
sync/zstackio/ZSV-12395
branch
2 times, most recently
from
898f83e to
5996dbf
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
header/src/main/java/org/zstack/header/core/external/service/APIUpdateExternalServiceConfigurationMsgDoc_zh_cn.groovy (1)
33-41:⚠️ Potential issue | 🟠 Major | ⚡ Quick win修正文档：configuration 字段不应作为可更新参数。
APIUpdateExternalServiceConfigurationMsgDoc_zh_cn.groovy将configuration字段（optional: true）暴露为更新入参；但根据ExternalServiceManagerImpl.updateExternalServiceConfiguration()的实现，Update API 仅支持更新description，configuration变更不被处理且不会触发applyExternalServiceConfigurationToAllNodes()。修改配置需要删除后重新创建（UUID 会变化）。建议移除该文档字段定义，或在字段
desc中明确标注：configuration当前不生效，仅可更新description，修改配置请走"删除+新增"流程。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@header/src/main/java/org/zstack/header/core/external/service/APIUpdateExternalServiceConfigurationMsgDoc_zh_cn.groovy` around lines 33 - 41, The doc wrongly exposes the "configuration" field as updatable in APIUpdateExternalServiceConfigurationMsgDoc_zh_cn.groovy while ExternalServiceManagerImpl.updateExternalServiceConfiguration() only processes "description" and does not call applyExternalServiceConfigurationToAllNodes(); remove the "configuration" column block from the update API doc or alternatively update its desc to state that configuration changes are not applied and that configuration must be changed by deleting and recreating the external service (UUID will change), ensuring references to APIUpdateExternalServiceConfigurationMsgDoc_zh_cn.groovy, ExternalServiceManagerImpl.updateExternalServiceConfiguration(), and applyExternalServiceConfigurationToAllNodes() are used to locate the logic and confirm correctness.
🧹 Nitpick comments (1)
header/src/main/java/org/zstack/header/core/external/service/ExternalServiceConfigurationVO.java (1)
23-24: ⚡ Quick win建议为 serviceType 字段添加显式长度约束。
数据库 schema 中
serviceType定义为varchar(32)（Line 140 in V5.1.0__schema.sql），但 Java 字段仅使用@Column注解未声明length属性。建议添加@Column(length = 32)以确保 JPA 层与数据库定义一致，避免运行时因超长值触发截断错误。🔧 建议的修复
- `@Column` + `@Column`(length = 32) private String serviceType;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@header/src/main/java/org/zstack/header/core/external/service/ExternalServiceConfigurationVO.java` around lines 23 - 24, The serviceType field in ExternalServiceConfigurationVO lacks an explicit length constraint causing a mismatch with the DB schema; update the field's annotation by adding a length attribute (i.e., change the `@Column` on the serviceType field in class ExternalServiceConfigurationVO to include length = 32) so JPA will enforce varchar(32) consistent with the database.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@header/src/main/java/org/zstack/header/core/external/service/APIUpdateExternalServiceConfigurationMsgDoc_zh_cn.groovy`:
- Around line 33-41: The doc wrongly exposes the "configuration" field as
updatable in APIUpdateExternalServiceConfigurationMsgDoc_zh_cn.groovy while
ExternalServiceManagerImpl.updateExternalServiceConfiguration() only processes
"description" and does not call applyExternalServiceConfigurationToAllNodes();
remove the "configuration" column block from the update API doc or alternatively
update its desc to state that configuration changes are not applied and that
configuration must be changed by deleting and recreating the external service
(UUID will change), ensuring references to
APIUpdateExternalServiceConfigurationMsgDoc_zh_cn.groovy,
ExternalServiceManagerImpl.updateExternalServiceConfiguration(), and
applyExternalServiceConfigurationToAllNodes() are used to locate the logic and
confirm correctness.
---
Nitpick comments:
In
`@header/src/main/java/org/zstack/header/core/external/service/ExternalServiceConfigurationVO.java`:
- Around line 23-24: The serviceType field in ExternalServiceConfigurationVO
lacks an explicit length constraint causing a mismatch with the DB schema;
update the field's annotation by adding a length attribute (i.e., change the
`@Column` on the serviceType field in class ExternalServiceConfigurationVO to
include length = 32) so JPA will enforce varchar(32) consistent with the
database.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: aab5ee25-5dc0-4867-87f5-0b0a0d983efe
⛔ Files ignored due to path filters (2)
conf/persistence.xmlis excluded by!**/*.xmlconf/serviceConfig/externalService.xmlis excluded by!**/*.xml
📒 Files selected for processing (40)
conf/db/zsv/V5.1.0__schema.sqlcore/src/main/java/org/zstack/core/externalservice/ExternalService.javacore/src/main/java/org/zstack/core/externalservice/ExternalServiceManagerImpl.javaheader/src/main/java/org/zstack/header/core/external/service/APIAddExternalServiceConfigurationEvent.javaheader/src/main/java/org/zstack/header/core/external/service/APIAddExternalServiceConfigurationEventDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/core/external/service/APIAddExternalServiceConfigurationMsg.javaheader/src/main/java/org/zstack/header/core/external/service/APIAddExternalServiceConfigurationMsgDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/core/external/service/APIDeleteExternalServiceConfigurationEvent.javaheader/src/main/java/org/zstack/header/core/external/service/APIDeleteExternalServiceConfigurationEventDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/core/external/service/APIDeleteExternalServiceConfigurationMsg.javaheader/src/main/java/org/zstack/header/core/external/service/APIDeleteExternalServiceConfigurationMsgDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/core/external/service/APIQueryExternalServiceConfigurationMsg.javaheader/src/main/java/org/zstack/header/core/external/service/APIQueryExternalServiceConfigurationMsgDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/core/external/service/APIQueryExternalServiceConfigurationReply.javaheader/src/main/java/org/zstack/header/core/external/service/APIQueryExternalServiceConfigurationReplyDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/core/external/service/APIUpdateExternalServiceConfigurationEvent.javaheader/src/main/java/org/zstack/header/core/external/service/APIUpdateExternalServiceConfigurationEventDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/core/external/service/APIUpdateExternalServiceConfigurationMsg.javaheader/src/main/java/org/zstack/header/core/external/service/APIUpdateExternalServiceConfigurationMsgDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/core/external/service/ApplyExternalConfigurationResult.javaheader/src/main/java/org/zstack/header/core/external/service/ApplyExternalServiceConfigurationMsg.javaheader/src/main/java/org/zstack/header/core/external/service/ApplyExternalServiceConfigurationReply.javaheader/src/main/java/org/zstack/header/core/external/service/ExternalServiceConfigurationInventory.javaheader/src/main/java/org/zstack/header/core/external/service/ExternalServiceConfigurationInventoryDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/core/external/service/ExternalServiceConfigurationVO.javaheader/src/main/java/org/zstack/header/core/external/service/ExternalServiceConfigurationVO_.javaheader/src/main/java/org/zstack/header/core/external/service/ExternalServiceInventory.javasdk/src/main/java/org/zstack/sdk/AddExternalServiceConfigurationAction.javasdk/src/main/java/org/zstack/sdk/AddExternalServiceConfigurationResult.javasdk/src/main/java/org/zstack/sdk/ApplyExternalConfigurationResult.javasdk/src/main/java/org/zstack/sdk/DeleteExternalServiceConfigurationAction.javasdk/src/main/java/org/zstack/sdk/DeleteExternalServiceConfigurationResult.javasdk/src/main/java/org/zstack/sdk/ExternalServiceConfigurationInventory.javasdk/src/main/java/org/zstack/sdk/ExternalServiceInventory.javasdk/src/main/java/org/zstack/sdk/QueryExternalServiceConfigurationAction.javasdk/src/main/java/org/zstack/sdk/QueryExternalServiceConfigurationResult.javasdk/src/main/java/org/zstack/sdk/SourceClassMap.javasdk/src/main/java/org/zstack/sdk/UpdateExternalServiceConfigurationAction.javasdk/src/main/java/org/zstack/sdk/UpdateExternalServiceConfigurationResult.javatestlib/src/main/java/org/zstack/testlib/ApiHelper.groovy
✅ Files skipped from review due to trivial changes (5)
- sdk/src/main/java/org/zstack/sdk/QueryExternalServiceConfigurationResult.java
- header/src/main/java/org/zstack/header/core/external/service/ExternalServiceConfigurationInventoryDoc_zh_cn.groovy
- header/src/main/java/org/zstack/header/core/external/service/ExternalServiceConfigurationVO_.java
- header/src/main/java/org/zstack/header/core/external/service/APIQueryExternalServiceConfigurationReplyDoc_zh_cn.groovy
- sdk/src/main/java/org/zstack/sdk/DeleteExternalServiceConfigurationResult.java
🚧 Files skipped from review as they are similar to previous changes (31)
- sdk/src/main/java/org/zstack/sdk/ExternalServiceInventory.java
- header/src/main/java/org/zstack/header/core/external/service/APIUpdateExternalServiceConfigurationEventDoc_zh_cn.groovy
- header/src/main/java/org/zstack/header/core/external/service/APIQueryExternalServiceConfigurationMsg.java
- header/src/main/java/org/zstack/header/core/external/service/APIDeleteExternalServiceConfigurationEvent.java
- header/src/main/java/org/zstack/header/core/external/service/ApplyExternalServiceConfigurationReply.java
- header/src/main/java/org/zstack/header/core/external/service/ApplyExternalServiceConfigurationMsg.java
- sdk/src/main/java/org/zstack/sdk/AddExternalServiceConfigurationResult.java
- sdk/src/main/java/org/zstack/sdk/ExternalServiceConfigurationInventory.java
- header/src/main/java/org/zstack/header/core/external/service/APIDeleteExternalServiceConfigurationEventDoc_zh_cn.groovy
- sdk/src/main/java/org/zstack/sdk/ApplyExternalConfigurationResult.java
- header/src/main/java/org/zstack/header/core/external/service/ApplyExternalConfigurationResult.java
- header/src/main/java/org/zstack/header/core/external/service/APIDeleteExternalServiceConfigurationMsgDoc_zh_cn.groovy
- header/src/main/java/org/zstack/header/core/external/service/APIUpdateExternalServiceConfigurationEvent.java
- header/src/main/java/org/zstack/header/core/external/service/ExternalServiceInventory.java
- core/src/main/java/org/zstack/core/externalservice/ExternalService.java
- header/src/main/java/org/zstack/header/core/external/service/APIAddExternalServiceConfigurationMsgDoc_zh_cn.groovy
- header/src/main/java/org/zstack/header/core/external/service/APIAddExternalServiceConfigurationEventDoc_zh_cn.groovy
- header/src/main/java/org/zstack/header/core/external/service/APIQueryExternalServiceConfigurationMsgDoc_zh_cn.groovy
- sdk/src/main/java/org/zstack/sdk/UpdateExternalServiceConfigurationResult.java
- header/src/main/java/org/zstack/header/core/external/service/APIAddExternalServiceConfigurationEvent.java
- sdk/src/main/java/org/zstack/sdk/SourceClassMap.java
- header/src/main/java/org/zstack/header/core/external/service/APIQueryExternalServiceConfigurationReply.java
- sdk/src/main/java/org/zstack/sdk/UpdateExternalServiceConfigurationAction.java
- header/src/main/java/org/zstack/header/core/external/service/APIAddExternalServiceConfigurationMsg.java
- sdk/src/main/java/org/zstack/sdk/DeleteExternalServiceConfigurationAction.java
- testlib/src/main/java/org/zstack/testlib/ApiHelper.groovy
- header/src/main/java/org/zstack/header/core/external/service/APIUpdateExternalServiceConfigurationMsg.java
- header/src/main/java/org/zstack/header/core/external/service/ExternalServiceConfigurationInventory.java
- sdk/src/main/java/org/zstack/sdk/QueryExternalServiceConfigurationAction.java
- core/src/main/java/org/zstack/core/externalservice/ExternalServiceManagerImpl.java
- sdk/src/main/java/org/zstack/sdk/AddExternalServiceConfigurationAction.java
MatheMatrix
force-pushed
the
sync/zstackio/ZSV-12395
branch
from
5996dbf to
750153d
Compare
- Add ExternalServiceConfigurationVO schema, inventory, query API, and add/update/delete APIs - Apply configuration changes to matching external services across management nodes - Expose serviceType in external service inventory for configuration routing - Generate SDK actions/results and testlib helpers for external service configuration operations - Register persistence and external service API metadata DBImpact APIImpact Resolves: ZSV-12395 Change-Id: I6a6b76626770736871716563676b6a7a74736966
MatheMatrix
force-pushed
the
sync/zstackio/ZSV-12395
branch
from
750153d to
37655fe
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DBImpact
APIImpact
Resolves: ZSV-12395
Change-Id: I6a6b76626770736871716563676b6a7a74736966
sync from gitlab !10144