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.

Sign up for GitHub

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

Jump to bottom

Nacos datasource #184

Merged
merged 19 commits into from
Jul 22, 2020
Merged

Nacos datasource #184

merged 19 commits into from
Jul 22, 2020

Conversation

binbin0325
Copy link
Collaborator

Describe what this PR does / why we need it

Add nacos datasource.

Does this pull request fix one issue?

Fixes #62

Describe how you did it

Implement dataSource interface.Get the remote configuration using https://github.com/nacos-group/nacos-sdk-go

Describe how to verify it

Execute unit tests.

Special notes for reviews

@CLAassistant
Copy link

CLAassistant commented Jul 13, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@sczyh30 sczyh30 added the to-review PRs to review label Jul 13, 2020
@sczyh30 sczyh30 requested review from sczyh30 and louyuting July 13, 2020 05:35
@sczyh30 sczyh30 added the area/data-source Issues or PRs related to data-source extension label Jul 13, 2020
@codecov-commenter
Copy link

codecov-commenter commented Jul 13, 2020 •
edited
Loading

Codecov Report

Merging #184 into master will increase coverage by 0.33%.
The diff coverage is 47.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #184      +/-   ##
==========================================
+ Coverage   44.47%   44.81%   +0.33%     
==========================================
  Files          76       80       +4     
  Lines        4209     4432     +223     
==========================================
+ Hits         1872     1986     +114     
- Misses       2132     2219      +87     
- Partials      205      227      +22
Impacted Files Coverage Δ
core/hotspot/rule.go 66.25% <0.00%> (+2.50%) ⬆️
core/hotspot/slot.go 33.33% <0.00%> (+0.37%) ⬆️
ext/datasource/nacos/nacos_example.go 26.92% <26.92%> (ø)
ext/datasource/consul/consul.go 39.06% <39.06%> (ø)
core/hotspot/traffic_shaping.go 63.05% <42.85%> (-1.53%) ⬇️
ext/datasource/nacos/nacos.go 57.40% <57.40%> (ø)
ext/datasource/consul/option.go 57.89% <57.89%> (ø)
api/slot_chain.go 70.58% <100.00%> (ø)
core/base/block_error.go 14.28% <100.00%> (+4.28%) ⬆️
core/hotspot/rule_manager.go 58.98% <100.00%> (+0.80%) ⬆️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd908a5...9a1b59c. Read the comment docs.

louyuting
louyuting requested changes Jul 15, 2020
View reviewed changes
ext/datasource/nacos/nacos.go Outdated Show resolved Hide resolved
ext/datasource/nacos/nacos.go Outdated Show resolved Hide resolved
@binbin0325 binbin0325 requested a review from louyuting July 20, 2020 00:25
@louyuting
Copy link
Collaborator

Hi @sanxun0325
I do a draft refactor, Could you please review this diff?
76fa7e0

@binbin0325
Copy link
Collaborator Author

@louyuting I understand your idea, and I will modify it according to your suggestion.thank you.

louyuting
louyuting previously approved these changes Jul 21, 2020
View reviewed changes
sczyh30
sczyh30 reviewed Jul 22, 2020
View reviewed changes
ext/datasource/nacos/nacos.go Outdated
closeChan chan struct{}
}

func NewNacosDataSource(client config_client.IConfigClient, propertyConfig vo.ConfigParam, handlers ...datasource.PropertyHandler) (*NacosDataSource, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could just replace vo.ConfigParam with dataId and groupId here? Using vo.ConfigParam might be confusing (e.g. the listener function inside the config is not actually used).

sczyh30
sczyh30 approved these changes Jul 22, 2020
View reviewed changes
Copy link
Member

@sczyh30 sczyh30 left a comment

Choose a reason for hiding this comment

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

LGTM

@sczyh30 sczyh30 merged commit d51b06d into alibaba:master Jul 22, 2020
@sczyh30
Copy link
Member

sczyh30 commented Jul 22, 2020

Thanks for contributing!

@sczyh30 sczyh30 removed the to-review PRs to review label Jul 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sczyh30 sczyh30 sczyh30 approved these changes

@louyuting louyuting louyuting left review comments

Assignees
No one assigned
Labels
area/data-source Issues or PRs related to data-source extension
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Nacos-go data-source extension
5 participants
@binbin0325 @CLAassistant @codecov-commenter @louyuting @sczyh30