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(registry): support discovery registry center #1480

Merged
merged 4 commits into from
Sep 28, 2021
Merged

Conversation

yeqown
Copy link
Contributor

@yeqown yeqown commented Sep 15, 2021

Description (what this PR does / why we need it):

  • support discovery registry center
  • add example for using discovery registry center

Which issue(s) this PR fixes (resolves / be part of):

resolves #1472

Other special notes for reviewer:

I need some time to refactor code style, any advices and code review can help this work, thanks ~

@codecov-commenter
Copy link

codecov-commenter commented Sep 15, 2021

Codecov Report

Merging #1480 (a00bcfe) into main (24ec23f) will increase coverage by 0.10%.
The diff coverage is n/a.

❗ Current head a00bcfe differs from pull request most recent head 65243a4. Consider uploading reports for the commit 65243a4 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1480      +/-   ##
==========================================
+ Coverage   66.65%   66.76%   +0.10%     
==========================================
  Files          66       66              
  Lines        3038     3033       -5     
==========================================
  Hits         2025     2025              
+ Misses        841      836       -5     
  Partials      172      172              
Impacted Files Coverage Δ
transport/http/calloption.go 100.00% <0.00%> (+15.62%) ⬆️

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 24ec23f...65243a4. Read the comment docs.

@daemon365
Copy link
Member

feat #1472

@yeqown
Copy link
Contributor Author

yeqown commented Sep 18, 2021

review plz ~

@longXboy
Copy link
Member

longXboy commented Sep 24, 2021

Why not quote the original bilibili/discovery SDK and wrap it to implement kratos interface instead of copying the code?

@yeqown
Copy link
Contributor Author

yeqown commented Sep 24, 2021

@longXboy

  1. The old package(discovery/naming) imports kratos@v1 package, it's not wise to use old version of kratos. https://github.com/bilibili/discovery/blob/1e12d5c0080ecd7ce97ab78076ef36dda8d56a1a/naming/client.go#L16-L19
  2. Personal preference: the old package use std http client which is no friendly to human and makes the code fetid and wordy.
  3. I'm not sure of that is it workable wraps bilibili/discovery SDK to implement kratos@v2 registry.Registrar and registry.Discovery interfaces.

@longXboy
Copy link
Member

@longXboy

  1. The old package(discovery/naming) imports kratos@v1 package, it's not wise to use old version of kratos. https://github.com/bilibili/discovery/blob/1e12d5c0080ecd7ce97ab78076ef36dda8d56a1a/naming/client.go#L16-L19
  2. Personal preference: the old package use std http client which is no friendly to human and makes the code fetid and wordy.
  3. I'm not sure of that is it workable wraps bilibili/discovery SDK to implement kratos@v2 registry.Registrar and registry.Discovery interfaces.

LGTM

@longXboy longXboy merged commit 5786f61 into go-kratos:main Sep 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] support discovery registry center
5 participants