Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: deep copy the configuration when instanceOptions flow to ServerOptions/ClientOptions #2596

Merged
merged 6 commits into from
Feb 29, 2024

Conversation

setcy
Copy link
Contributor

@setcy setcy commented Feb 14, 2024

…Options/ClientOptions

Fixes: apache#2592

Signed-off-by: setcy <asetcy@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Feb 14, 2024

Codecov Report

Attention: 139 lines in your changes are missing coverage. Please review.

Comparison is base (198504b) 47.55% compared to head (ce8fddf) 47.62%.
Report is 1 commits behind head on main.

Files Patch % Lines
options.go 29.03% 36 Missing and 8 partials ⚠️
global/metric_config.go 60.86% 17 Missing and 10 partials ⚠️
global/service_config.go 80.00% 5 Missing and 5 partials ⚠️
global/reference_config.go 77.50% 5 Missing and 4 partials ⚠️
global/otel_config.go 63.15% 4 Missing and 3 partials ⚠️
global/provider_config.go 76.92% 3 Missing and 3 partials ⚠️
global/config_center_config.go 78.94% 2 Missing and 2 partials ⚠️
global/consumer_config.go 80.95% 2 Missing and 2 partials ⚠️
global/custom_config.go 60.00% 2 Missing and 2 partials ⚠️
global/logger_config.go 81.81% 2 Missing and 2 partials ⚠️
... and 8 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2596      +/-   ##
==========================================
+ Coverage   47.55%   47.62%   +0.06%     
==========================================
  Files         318      339      +21     
  Lines       23627    24925    +1298     
==========================================
+ Hits        11236    11870     +634     
- Misses      11328    11902     +574     
- Partials     1063     1153      +90     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: setcy <asetcy@gmail.com>
Signed-off-by: setcy <asetcy@gmail.com>
global/tls_config.go Outdated Show resolved Hide resolved
Signed-off-by: setcy <asetcy@gmail.com>
Signed-off-by: setcy <asetcy@gmail.com>
@setcy setcy marked this pull request as ready for review February 18, 2024 09:19
Copy link
Contributor

@chickenlj chickenlj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Contributor

@DMwangnima DMwangnima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice Job @setcy !
There are some points that we could discuss:

  1. Clone relys on handwriting individual fields. It is easy to miss corresponding clone procedure when we add fields. I think we should add test to ensure that the missing would not happen.
  2. The issue had illustrated the modifying problem. Maybe we could add a test in dubbo_test.go to cover this scenario?(i.e. Modify some parts of configuration in Client/Server layer, and the configurations in upper layer did not be modified)

Signed-off-by: setcy <asetcy@gmail.com>
Copy link

sonarcloud bot commented Feb 21, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@chickenlj chickenlj merged commit 8efb9ea into apache:main Feb 29, 2024
5 checks passed
FoghostCn pushed a commit to FoghostCn/dubbo-go that referenced this pull request Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants