Skip to content

Conversation

WeihanLi
Copy link
Contributor

  • Fix the condition
  • Remove System.Text.Json dependency for .NET 6, still required for .NET 5 because of new json features introduced in 6.0.0

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: WeihanLi
To complete the pull request process, please assign brendandburns after the PR has been reviewed.
You can assign the PR to them by writing /assign @brendandburns in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 27, 2022
@tg123
Copy link
Member

tg123 commented Feb 27, 2022

The library is built-in as part of the shared framework for .NET Core 3.0 and later versions
https://docs.microsoft.com/en-us/dotnet/standard/serialization/system-text-json-overview?pivots=dotnet-5-0#how-to-get-the-library

@WeihanLi
Copy link
Contributor Author

The library is built-in as part of the shared framework for .NET Core 3.0 and later versions
https://docs.microsoft.com/en-us/dotnet/standard/serialization/system-text-json-overview?pivots=dotnet-5-0#how-to-get-the-library

The build would fail if remove the dependency for .NET 5, the built-in version seemed is not latest version

@tg123
Copy link
Member

tg123 commented Feb 27, 2022

tests proj targets net5 as well, could you please elaborate yours

@WeihanLi
Copy link
Contributor Author

WeihanLi commented Feb 27, 2022

The condition did not work before because of the condition typo

- Coondition
+ Condition

And it can be verified from the NuGet package dependencies

1645950745(1)

@tg123
Copy link
Member

tg123 commented Feb 28, 2022

lol this is very interesting.
the coondition here was intent to make net5+ use their own builtin json library instead of a dependency
it equals to no condition today

i tested fixing the typo and found serval build errors and understand the issue eventually.

the correct way, i believe, to fix it is to fix the typo and workaround those features in .6
OR
change to condition to ==netstandard2.1 or ==net5 in case of adopting net7

seems the second one is easier as netcore3.1 and net5 are approaching the eol

@tg123
Copy link
Member

tg123 commented Feb 28, 2022

thanks for the PR

/LGTM

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 28, 2022
@tg123 tg123 merged commit c11e205 into kubernetes-client:master Feb 28, 2022
@WeihanLi WeihanLi deleted the clean-dependencies branch February 28, 2022 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants