Conversation
|
wemjie xiao seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
I've signed the CLA ( Details:
Thanks for your help! |
|
commit 中和你本地的邮箱不同,可以检查下看看 |
There was a problem hiding this comment.
Pull request overview
This PR attempts to demonstrate simplified Seata configuration by removing redundant YAML properties and relying on defaults. However, it adds an application.yml file to the nacos-config-test module, which is designed specifically for testing Nacos configuration functionality, not Seata.
Key Issues:
- The configuration is added to the wrong module (nacos-config-test has no Seata dependencies)
- YAML syntax error with duplicate top-level 'spring' keys that will cause configuration loss
- Mix of active configuration and extensive commented-out documentation examples
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| password: nacos | ||
|
|
||
| # 完全删除以下冗余配置: | ||
| # seata: | ||
| # config: | ||
| # type: nacos | ||
| # nacos: | ||
| # serverAddr: 127.0.0.1:8848 | ||
| # dataId: "seata.properties" | ||
| # username: nacos | ||
| # password: nacos | ||
| # registry: | ||
| # type: nacos | ||
| # nacos: | ||
| # application: seata-server | ||
| # server-addr: 127.0.0.1:8848 | ||
| # username: nacos | ||
| # password: nacos No newline at end of file |
There was a problem hiding this comment.
The commented-out configuration section (lines 22-37) appears to be documentation rather than actual test configuration. Test configuration files should contain only the minimal, valid configuration needed to run the tests. Including large blocks of commented-out "example" configuration makes the file harder to maintain and understand. If the goal is to provide configuration examples, they should be in documentation files or separate example projects, not in test resource files.
| password: nacos | |
| # 完全删除以下冗余配置: | |
| # seata: | |
| # config: | |
| # type: nacos | |
| # nacos: | |
| # serverAddr: 127.0.0.1:8848 | |
| # dataId: "seata.properties" | |
| # username: nacos | |
| # password: nacos | |
| # registry: | |
| # type: nacos | |
| # nacos: | |
| # application: seata-server | |
| # server-addr: 127.0.0.1:8848 | |
| # username: nacos | |
| # password: nacos | |
| password: nacos |
| tx-service-group: ${spring.application.name}-tx-group # 可删除(默认行为) | ||
|
|
||
| # 如果已配置 Nacos 作为注册中心和配置中心,以下 Seata 配置可完全省略! | ||
| spring: |
There was a problem hiding this comment.
The YAML file has duplicate top-level 'spring' keys (lines 1 and 10). In YAML, duplicate keys at the same level will cause the first key to be overridden by the second one. This means the configuration at lines 1-3 (spring.application.name: business-service) will be lost. These configurations should be merged into a single 'spring' section.
| seata: | ||
| enabled: true # 启用 Seata | ||
| tx-service-group: ${spring.application.name}-tx-group # 可删除(默认行为) |
There was a problem hiding this comment.
This Seata configuration is being added to a Nacos config test module that has no Seata dependencies in its pom.xml. The module is specifically for testing Nacos configuration functionality (as evidenced by the test files like NacosConfigRefreshTest, NacosConfigurationTests, etc.). Adding Seata configuration here is misleading and doesn't align with the module's purpose. If the goal is to demonstrate Seata configuration simplification, this should be in a separate Seata-specific test module or example.
| @@ -0,0 +1,37 @@ | |||
| spring: | |||
| application: | |||
| name: business-service # 应用名,Seata 默认基于此生成事务组 | |||
There was a problem hiding this comment.
The application name 'business-service' is inappropriate for a Nacos configuration test module. This name suggests a business domain service rather than a test application. The existing test files use 'myTestService1' or similar test-oriented names. Consider using a name that clearly indicates this is a test application, such as 'nacos-config-test' to match the module's artifactId.
| name: business-service # 应用名,Seata 默认基于此生成事务组 | |
| name: nacos-config-test # 应用名,Seata 默认基于此生成事务组 |
| @@ -0,0 +1,37 @@ | |||
| spring: | |||
| application: | |||
| name: business-service # 应用名,Seata 默认基于此生成事务组 | |||
There was a problem hiding this comment.
This comment in Chinese states "Application name, Seata defaults to generating transaction group based on this", but this is a Nacos config test module without Seata dependencies. The comment is misleading and not relevant to the actual purpose of this test module. If Nacos-specific configuration documentation is needed, it should explain how the application name is used in Nacos config contexts.
| name: business-service # 应用名,Seata 默认基于此生成事务组 | |
| name: business-service # 应用名 / serviceId,在 Nacos 注册与配置中使用 |
|
|
||
| seata: | ||
| enabled: true # 启用 Seata | ||
| tx-service-group: ${spring.application.name}-tx-group # 可删除(默认行为) |
There was a problem hiding this comment.
The comment states "可删除(默认行为)" (Can be deleted - default behavior), which suggests this line is optional/redundant. If this configuration truly represents default behavior and can be deleted, it should not be included in the example configuration file at all. Including optional configurations with notes that they can be deleted creates confusion and goes against the PR's stated goal of removing redundant configuration.
| tx-service-group: ${spring.application.name}-tx-group # 可删除(默认行为) |
| enabled: true # 启用 Seata | ||
| tx-service-group: ${spring.application.name}-tx-group # 可删除(默认行为) | ||
|
|
||
| # 如果已配置 Nacos 作为注册中心和配置中心,以下 Seata 配置可完全省略! |
There was a problem hiding this comment.
This comment states "如果已配置 Nacos 作为注册中心和配置中心,以下 Seata 配置可完全省略!" (If Nacos is configured as registry and config center, the following Seata configuration can be completely omitted!). However, this is misleading because the Seata configuration section (lines 23-37) is already commented out. The comment suggests it's explaining what can be omitted, but it creates confusion about what is actually active configuration versus documentation. This entire documentation would be better suited for a separate example or documentation file rather than a test configuration.


Hi everyone,
This is my first Pull Request, and I'm a sophomore in Beijing University of Posts and Telecommunications,I'm still learning a lot , but I'm excited to contribute to this project, maybe there will be some problems, following is what I had done as the suggestions given by this table:
Describe what this PR does / why we need it
Does this pull request fix one issue?
Fixes #3756
Describe how you did it
Removed duplicate configs: Eliminated redundant properties in seata.config and seata.registry that overlap with global Nacos settings.
Leveraged defaults: Relied on Seata’s default behaviors (e.g., auto-generated transaction group names, seata-server service discovery).
Kept critical configs: Only retained seata.enabled=true to ensure functionality.
Describe how to verify it
Start dependencies: Ensure Nacos and Seata Server are running.
Test transactions: Invoke a method annotated with @GlobalTransactional to verify distributed transactions.
Log inspection: Check Seata client/server logs for errors related to missing configurations.
Special notes for reviews
Version compatibility: Confirm compatibility between Spring Cloud Alibaba and Seata (e.g., 2021.0.x + Seata 1.5.x).
Custom needs: Explicit configurations are required if using non-default DataId or service names.
Thanks for your time and support !
Best regards,
qichen012