rajanadar / VaultSharp

A comprehensive cross-platform .NET Library for HashiCorp's Vault, a secret management tool
http://rajanadar.github.io/VaultSharp
Apache License 2.0
493 stars 134 forks source link

Do you have plans to switch to System.Text.Json? #304

Closed vadimkhm closed 1 year ago

vadimkhm commented 1 year ago

In our projects, we are using only System.Text.Json, and it is disallowed to have a different parser, it would be great to see that VaultSharp switched to System.Text.Json

Thanks

rajanadar commented 1 year ago

hi @vadimkhm , let me assess the impact of that change and get back to you

rajanadar commented 1 year ago

i did an assessment @vadimkhm , it is a 2300 place change with regression testing. It also needs me to stop supporting anything before .NET 4.7.2 and .NET 1.3

That may break a lot of the users

let me think through and let you know.

vadimkhm commented 1 year ago

Hi, @rajanadar Thank you for the quick response.

Yes I checked too, it is not a slight shift, but considering that even Net Core 3.1 reached the end of life in 2022 and .NET Standard 1.3 has reached the end of life too, is there a reason to keep them?

rajanadar commented 1 year ago

Yeah makes. Sense, I have a branch in progress with the changes.

Let me finish the changes and by then I'll also make a call to deprecate the old versions.

I'll provide an update by next weekend.

rajanadar commented 1 year ago

I made the 2300 odd changes. Build is successful. Need to test the use cases over next week.

https://github.com/rajanadar/VaultSharp/pull/306

vadimkhm commented 1 year ago

Much appreciated! In case you need a helping hand, please let me know. I will be glad to assist.

hoerup commented 1 year ago

Before merging - consider the fact that it will break support for everything before net framework 4.7.2 as @rajanadar mentioned but that 4.6.2 is not EOL until 2027 and 4.7.0/4.7.1 doesn't yet have a EOL date defined

https://learn.microsoft.com/en-us/lifecycle/products/microsoft-net-framework

rajanadar commented 1 year ago

Yes @hoerup.

There are a lot of test failures. Once I fix those, I'll probably release a beta nuget package for @vadimkhm.

It's too many breaking changes for the master pipeline of the packages.

rajanadar commented 1 year ago

Much appreciated! In case you need a helping hand, please let me know. I will be glad to assist.

Thanks @vadimkhm. It's fine. I have some momentum, making steady progress.

vadimkhm commented 1 year ago

Before merging - consider the fact that it will break support for everything before net framework 4.7.2 as @rajanadar mentioned but that 4.6.2 is not EOL until 2027 and 4.7.0/4.7.1 doesn't yet have a EOL date defined

https://learn.microsoft.com/en-us/lifecycle/products/microsoft-net-framework

You are right about these versions, but it is very easy to switch from them to 4.7.2, we did the same and moved to 4.8 from 4.6 without any issues. The converted project was not a small one. But in the end, I can do a fork of @rajanadar's created branch and use it.

rajanadar commented 1 year ago

@vadimkhm You can see the progress on this branch btw.. Resolving one issue after the other..

https://github.com/rajanadar/VaultSharp/commits/sys-text-json-only

rajanadar commented 1 year ago

@hoerup What are your thoughts, if we remove support for the EOL versions only? Not for others.

So we would still support 4.6.2, 4.7, 4.7.1, 4.7.2, 4.8, 4.8.1, .NET Standard 2.0, .NET Standard 2.1, .NET 6.0 and .NET 7.0 And support would be removed for the following EOL versions: 4.6, 4.6.1, .NET Standard 1.3 and .NET 5.0

rajanadar commented 1 year ago

@vadimkhm I have published the official version of VaultSharp with this change. No more Newtonsoft Json dependency. Feel free to take that update and let me know if any issues.

https://www.nuget.org/packages/VaultSharp/1.13.0

vadimkhm commented 1 year ago

@rajanadar cool, thank you very much!

rajanadar commented 1 year ago

Thanks @vadimkhm for pointing me to this change. I think it's a good change for VaultSharp.

And hopefully, any future issues with json due to this change can be fix-forwards instead of rollbacks. Lets see.