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

[#1527 #1529] Breaking change to the class renaming of Kube service discovery provider #1954

Merged
merged 7 commits into from
Feb 12, 2024

Conversation

ZisisTsatsas
Copy link
Contributor

@ZisisTsatsas ZisisTsatsas commented Feb 2, 2024

@raman-m
Copy link
Member

raman-m commented Feb 2, 2024

@ZisisTsatsas Hi Zisis!
Thanks for the issue reporting!

You are right, the docs were not updated.
But it was my mistake when I renamed the class a half of the year ago... Seems we need to rename the class back to Kube.
So, finally there should be old two types: Kube and PollKube... and docs can be unchanged.
Sorry, for this breaking change!

@ggnaegi Do you agree using old types Kube and PollKube? But I know you don't use these providers...

@raman-m raman-m self-assigned this Feb 2, 2024
@raman-m raman-m added bug Identified as a potential bug documentation Needs a documentation update Service Discovery Ocelot feature: Service Discovery Nov'23 November 2023 release labels Feb 2, 2024
@raman-m raman-m added this to the Nov-December'23 milestone Feb 2, 2024
@raman-m
Copy link
Member

raman-m commented Feb 2, 2024

@ZisisTsatsas Could you help me with resolving this breaking change plz?
Or, I will work on the issue this weekend... Sorry, I'm very busy today and will be offline right away.

@raman-m raman-m changed the title Update kubernetes.rst Breaking change of renaming of Kube service discovery provider Feb 6, 2024
@raman-m raman-m changed the title Breaking change of renaming of Kube service discovery provider Breaking change to the class renaming of Kube service discovery provider Feb 6, 2024
The `Type` field for the kubernetes example is outdated
@raman-m
Copy link
Member

raman-m commented Feb 8, 2024

The root cause of the problem:

if (provider.GetType().Name.ToLower() == config.Type.ToLower())

of these block of code:

if (_delegates != null)
{
var provider = _delegates?.Invoke(_provider, config, route);
if (provider.GetType().Name.ToLower() == config.Type.ToLower())
{
return new OkResponse<IServiceDiscoveryProvider>(provider);
}
}

where _delegates is ServiceDiscoveryFinderDelegate and it is added into the DI:

.AddSingleton(KubernetesProviderFactory.Get)

Finally, in this line of code we have

if (provider.GetType().Name.ToLower() == config.Type.ToLower())

if ("KubernetesServiceDiscoveryProvider".ToLower() == "Kube".ToLower()) which is false always!
This is the bug and the breaking change ❗

@raman-m raman-m added accepted Bug or feature would be accepted as a PR or is being worked on Kubernetes Service discovery by Kubernetes labels Feb 8, 2024
@raman-m
Copy link
Member

raman-m commented Feb 10, 2024

Hi @ZisisTsatsas !
I found a couple of issues related to Kube provider and attached them to your PR 👉

So, these bugs will be closed officially by your PR 😉

@ggnaegi FYI

@raman-m raman-m changed the title Breaking change to the class renaming of Kube service discovery provider (#1527 #1529) Breaking change to the class renaming of Kube service discovery provider Feb 10, 2024
@raman-m
Copy link
Member

raman-m commented Feb 10, 2024

Development completed ✔️

@RaynaldM @ggnaegi Please review!

@raman-m raman-m changed the title (#1527 #1529) Breaking change to the class renaming of Kube service discovery provider [#1527 #1529] Breaking change to the class renaming of Kube service discovery provider Feb 10, 2024
@raman-m raman-m mentioned this pull request Feb 11, 2024
Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

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

Approved by @ggnaegi

The build is 🟢

Ready for delivery! ✔️

@raman-m raman-m merged commit a1607f5 into ThreeMammals:develop Feb 12, 2024
2 checks passed
@raman-m raman-m removed the accepted Bug or feature would be accepted as a PR or is being worked on label Feb 12, 2024
@raman-m
Copy link
Member

raman-m commented Feb 12, 2024

Funny fact is that the breaking change was introduced in 15.0.0 release by Tom in the commit 6e5471a 🤯
I thought that I was the author of the bug in 19.x or 20.0 releases 🤣 but No: Tom did the thing. 😄

@raman-m
Copy link
Member

raman-m commented Feb 12, 2024

@ZisisTsatsas
Will you use our updated & fixed Kubernetes provider and Ocelot after the release? 😉

@ZisisTsatsas
Copy link
Contributor Author

@ZisisTsatsas Will you use our updated & fixed Kubernetes provider and Ocelot after the release? 😉

I will be using it! Thanks for the quick udpate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Identified as a potential bug documentation Needs a documentation update Kubernetes Service discovery by Kubernetes Nov'23 November 2023 release Service Discovery Ocelot feature: Service Discovery
Projects
None yet
3 participants