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

#1225 Update ServiceDiscovery documentation and samples to include Custom Providers #1656

Merged
merged 28 commits into from
Sep 28, 2023
Merged

#1225 Update ServiceDiscovery documentation and samples to include Custom Providers #1656

merged 28 commits into from
Sep 28, 2023

Conversation

leonluc-dev
Copy link
Contributor

@leonluc-dev leonluc-dev commented May 22, 2023 •
edited by raman-m

Closes #1225

The new section in service discovery documentation on how to implement a custom service provider.

Proposed Changes

  • Add "Custom Providers" section to Service Discovery documentation
  • Add new OcelotServiceDiscovery sample with initial solution of custom service discovery provider

@raman-m raman-m added the documentation Needs a documentation update label May 22, 2023
@raman-m raman-m self-requested a review May 22, 2023 18:59
@raman-m
Copy link
Member

raman-m commented May 22, 2023

Hi @leonluc-dev ! Leon or Leonluc?
So great to see that people working on writing docs! 🤣
I will check tomorrow...

@raman-m
Copy link
Member

raman-m commented May 24, 2023

Hey @leonluc-dev !
Did you check current open issues and maybe there is some issue which is related to custom ServiceDiscovery providers?
It would be highly welcomed to attach an issue to this PR.

raman-m
raman-m previously approved these changes May 24, 2023
View reviewed changes
@raman-m
Copy link
Member

raman-m commented May 24, 2023

@leonluc-dev
I expect that you have some real working solution.
Did you write some unit tests and/or acceptance tests for this "Custom service discovery providers" feature?

@leonluc-dev
Copy link
Contributor Author

leonluc-dev commented May 24, 2023 •
edited by raman-m

@raman-m Thank you for the response.

I made no modifications to the Ocelot code itself. What I've documented was an already present and tested Ocelot interface (IServiceDiscoveryProvider) I've been using in my own code for a while now using the public Ocelot NuGet packages.

Most if not all of the Ocelot code this documentation example utilizes is already unit tested through the ServiceDiscoveryProviderFactory and related tests: ServiceDiscoveryProviderFactoryTests

While there have been no Github issues directly related to this particular interface, there have been questions about custom service discovery in the past (example: #1225)

If this interface/code was not meant to be used in that sense then this PR can be closed without merge.

Otherwise I do have a working project where I utilize a custom service discovery provider. I could turn that into a proper sample and add it to samples if necessary? (I should have a bit of time for that later this or next week)

@raman-m
Copy link
Member

raman-m commented May 24, 2023 •
edited

@leonluc-dev
Thanks for the reply!

Otherwise I do have a working project where I utilize a custom service discovery provider. I could turn that into a proper sample and add it to samples if necessary? (I should have a bit of time for that later this or next week)

It is not required, but it will be highly welcomed to add.

Regarding tests. Yes, current tests should cover custom provider.
I am not sure the example in docs is canonic. But, real working solution in samples will be very useful.
In the future we can link a Sample app from this doc to show how to build custom providers.

Once again,
Thanks for sharing of your practical case!
That's how to make Ocelot customizable, even in such kind of critical features of Ocelot core.

@raman-m raman-m linked an issue May 24, 2023 that may be closed by this pull request
Is Custom Service Discovery or Dynamic Route Registration Supported? #1225
Closed
@raman-m raman-m added the accepted Bug or feature would be accepted as a PR or is being worked on label May 24, 2023
@raman-m raman-m changed the title Update servicediscovery documentation to include custom provider Update ServiceDiscovery documentation with the Custom Providers paragraph May 24, 2023
@raman-m raman-m requested a review from TomPallister May 24, 2023 15:34
@raman-m raman-m mentioned this pull request May 24, 2023
Closed
@leonluc-dev leonluc-dev changed the title Update ServiceDiscovery documentation with the Custom Providers paragraph Update ServiceDiscovery documentation and samples to include Custom Providers May 25, 2023
@leonluc-dev
Copy link
Contributor Author

@ramam-m I have added the sample for custom service discovery providers to the PR.

@raman-m
Copy link
Member

raman-m commented May 26, 2023

It was bad idea to include the new sample project right to this PR which was about docs updating.
Now we have broken pipelines and I need to review once again.
It was better to create separate PR. Well... Now it is too late. I forgot about asking you to prepare second PR...

Leon, you should expect that this PR will have long story...

@raman-m raman-m added feature A new feature medium effort Likely a few days of development effort proposal Proposal for a new functionality in Ocelot needs validation Issue has not been replicated or verified yet and removed accepted Bug or feature would be accepted as a PR or is being worked on documentation Needs a documentation update labels May 26, 2023
@raman-m raman-m requested review from raman-m and removed request for TomPallister May 26, 2023 09:20
@leonluc-dev
Copy link
Contributor Author

leonluc-dev commented May 26, 2023 •
edited

@raman-m Apologies for the misunderstanding.

If it makes it easier, I can rebase the branch and remove the commit involving the sample from this PR. Afterwards I can add the sample to a new seperate PR.

@raman-m
Copy link
Member

raman-m commented May 26, 2023

Leon,
Do nothing with this PR. I've started code review...

And just FYI, your develop branch is not canonic now. And your source repo develop branch should have the same top commits in target/base branch. You will be blocked in creation of new feature branches, because both develop branches must be synchronized always.
Hope you see...

raman-m
raman-m requested changes May 26, 2023
View reviewed changes
Copy link
Member

@raman-m raman-m left a comment •
edited

Choose a reason for hiding this comment

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

You've saved all code in the samples/OcelotCustom/ folder.
But what is custom?
The current folder structure use exact marker of an environment.

Please move all code to folder samples/OcelotServiceDiscovery/!

Second,
I don't see your sample solution in VS solution folders!
image

You need to add your sample to samples solution folder as service-discovery solution folder. Use eureka folder as an example.

So, your projects should be added to Ocelot.sln file!
That's how your projects will be compiled and checked by CI-CD pipelines.

samples/OcelotCustom/OcelotCustom.sln Outdated Show resolved Hide resolved
@leonluc-dev
Copy link
Contributor Author

leonluc-dev commented May 31, 2023 •
edited

@raman-m The requested changes should be addressed now.

@raman-m raman-m added documentation Needs a documentation update and removed needs validation Issue has not been replicated or verified yet labels Sep 8, 2023
@raman-m
Copy link
Member

raman-m commented Sep 8, 2023 •
edited

@leonluc-dev
The feature branch has been rebased onto ThreeMammals:develop!
Welcome to code review!

@raman-m raman-m added needs feedback Issue is waiting on feedback before acceptance and removed feature A new feature labels Sep 8, 2023
@raman-m
Copy link
Member

raman-m commented Sep 8, 2023 •
edited

@schwede Hi Swede!
Could you look at this PR and review the sample please?
Do you like it?
Did you get answers on your questions from original your issue?

@raman-m raman-m changed the title Update ServiceDiscovery documentation and samples to include Custom Providers #1225 Update ServiceDiscovery documentation and samples to include Custom Providers Sep 8, 2023
RaynaldM
RaynaldM previously approved these changes Sep 18, 2023
View reviewed changes
@raman-m raman-m added accepted Bug or feature would be accepted as a PR or is being worked on and removed medium effort Likely a few days of development effort needs feedback Issue is waiting on feedback before acceptance labels Sep 28, 2023
@raman-m raman-m merged commit 190b001 into ThreeMammals:develop Sep 28, 2023
2 checks passed
@raman-m
Copy link
Member

raman-m commented Sep 28, 2023

@leonluc-dev Congrats! 🎉

@raman-m raman-m mentioned this pull request Oct 3, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RaynaldM RaynaldM RaynaldM approved these changes

@raman-m raman-m raman-m approved these changes

@thiagoloureiro thiagoloureiro Awaiting requested review from thiagoloureiro

@TomPallister TomPallister Awaiting requested review from TomPallister

@binarymash binarymash Awaiting requested review from binarymash

@jps jps Awaiting requested review from jps

@geffzhang geffzhang Awaiting requested review from geffzhang

@philproctor philproctor Awaiting requested review from philproctor

@anktsrkr anktsrkr Awaiting requested review from anktsrkr

@sharpn sharpn Awaiting requested review from sharpn

@abooraja abooraja Awaiting requested review from abooraja

@wast wast Awaiting requested review from wast

@yangzhongke yangzhongke Awaiting requested review from yangzhongke

Assignees

@leonluc-dev leonluc-dev

Labels
accepted Bug or feature would be accepted as a PR or is being worked on documentation Needs a documentation update proposal Proposal for a new functionality in Ocelot
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Is Custom Service Discovery or Dynamic Route Registration Supported?
3 participants
@leonluc-dev @raman-m @RaynaldM