search

kubernetes-client / csharp

Officially supported dotnet Kubernetes Client library
Apache License 2.0
1.1k stars 295 forks source link

#if NETSTANDARD2_0 Preprocessor tags in KubernetesClientConfig #450

Closed KyleJFischer closed 4 years ago

KyleJFischer commented 4 years ago

I realize this might not be the best spot to ask this, but I figure it case anyone else has a similar issue then they might be able to get an answer:

I am curious on why "#if NETSTANDARD2_0" are placed around the external execution part of the code config build process (src/KubernetesClient/KubernetesClientConfig line 381). I am writing a .net core 3.1 application and currently it is skipping this section of code due to it not being .net standard 2.0. When I took the source code for the library and removed the #IF NETSTANDARD2_0 Lines it seemed to work.

I am curious to make sure I am not missing something that would break by removing them. Any information on why they are there, let me know!

tg123 commented 4 years ago

KuberneteClient sdk is multi targeting (netcore21 netstandard2 net452)

when you are doing .netcore31 you are using netcore21, bc it is more close to netcore31 than netstandard21

the reason for multitargeting is that netstandard2 does not support some API for websocket.

those things would be unified after targeting upgraded to netstandard2.1 however, only netcore3 supports it, so the code would be still there for a long time till netcore2.1 LTS retired.

brendandburns commented 4 years ago

I think we should remove this particular ifdef. I think it might be to remove Xamarin or some other framework, but I think it is safe to remove this today.

KyleJFischer commented 4 years ago

I can create a pull request to remove them if you want.

brendandburns commented 4 years ago

Yes, that'd be great. thanks

NasAmin commented 4 years ago

@KyleJFischer yes please create a pull request. It will be very much appreciated 👍

KyleJFischer commented 4 years ago

Pull request #451 has been made! Let me know what other changes if any you need me to do!

brendandburns commented 4 years ago

The PR merged, so I'm closing this issue.

NasAmin commented 4 years ago

@brendanburns Has the nuget package been published from this PR?

tg123 commented 4 years ago

@brendanburns Has the nuget package been published from this PR?

yes nuget will be pushed every time when master changed

check the latest one