feat: support Nacos v2 as ServiceRegistry (2/n) #718
Conversation
github-actions
bot
added
the
size/L
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #718 +/- ##
==========================================
+ Coverage 87.94% 88.40% +0.45%
==========================================
Files 128 128
Lines 6379 6433 +54
==========================================
+ Hits 5610 5687 +77
+ Misses 553 534 -19
+ Partials 216 212 -4 ☔ View full report in Codecov by Sentry. |
|
@spacewander the integration test failed because data race: https://github.com/mosn/htnn/actions/runs/10852269173/job/30117917532?pr=718 |
github-actions
bot
added
size/XL
We can add a comment on the corresponding test to link to that issue. |
| mosniov1 "mosn.io/htnn/types/apis/v1" | ||
| ) | ||
|
|
||
| func enableNacosV2(nacosInstance string) { |
There was a problem hiding this comment.
We can reuse the enableNacos?
| } | ||
| } | ||
|
|
||
| func disableNacosV2(nacosInstance string) { |
| params.Set("metadata", string(b)) | ||
| } | ||
|
|
||
| fullURL := nacosServerURL + "/nacos/v2/ns/instance?" + params.Encode() |
There was a problem hiding this comment.
We can make the ver as an argument?
| @@ -19,6 +19,28 @@ services: | |||
| - "8849:8848" | |||
| networks: | |||
| service: | |||
| nacos3: | |||
There was a problem hiding this comment.
Please add v2 in the service name so people can know the difference
github-actions
bot
added
size/L
|
|
||
| }) | ||
|
|
||
| // Nacos v2 sdk have bug about data race when creating a new nacos client |
There was a problem hiding this comment.
| // Nacos v2 sdk have bug about data race when creating a new nacos client | |
| // Nacos v2 sdk has a bug about data race when creating a new nacos client which might cause the test to fail |
| }) | ||
|
|
||
| _, err = NewClient(config) | ||
| assert.Error(t, err) |
There was a problem hiding this comment.
The test failed because of the bug of gomonkey when running on mac:
--- FAIL: TestNewClient (0.00s)
config_test.go:53:
Error Trace: /Users/lzx/git/htnn/controller/registries/nacos/v1/config_test.go:53
Error: An error is expected but got nil.
Test: TestNewClient
FAIL
coverage: 16.2% of statements in mosn.io/htnn/...
FAIL mosn.io/htnn/controller/registries/nacos/v1 7.662s
--- FAIL: TestNewClient (0.00s)
config_test.go:58:
Error Trace: /Users/lzx/git/htnn/controller/registries/nacos/v2/config_test.go:58
Error: An error is expected but got nil.
Test: TestNewClient
FAIL
coverage: 19.7% of statements in mosn.io/htnn/...
FAIL mosn.io/htnn/controller/registries/nacos/v2 7.695s
Would you drop the monkey patch like c0c4192?
|
👋 Hi, I detected conflicts against the base branch 🙊 |
ref #258