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

Switch to System.Text.Json #158

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Switch to System.Text.Json #158

wants to merge 7 commits into from

Conversation

mfkl
Copy link
Contributor

@mfkl mfkl commented Jul 14, 2022

Rebased and iterated on #73

Few points to discuss:

  • Bundled a few needed ConfigurationAwait(false) in the initial commit.
  • which version of Text.Json do we want? This comment is on point but over a year passed, so we can probably pick a reasonable version now.
  • Marcin dynamic comment,
  • do we want to keep a netcoreapp2.1 target? f042deb

Closes #43

@mfkl
Copy link
Contributor Author

mfkl commented Jul 14, 2022

netcoreapp2.1 failures are expected but there is a net461 timing one.

[xUnit.net 00:01:01.68]     Consul.Test.LockTest.Lock_ContendFast [FAIL]
Error: Consul.Test.LockTest.Lock_ContendFast: Consul.ConsulRequestException : Unexpected response, status code InternalServerError: invalid session "3103bd01-8165-4b14-cddd-542796ae036a"
  Failed Consul.Test.LockTest.Lock_ContendFast [31 s]
  Error Message:
   Consul.ConsulRequestException : Unexpected response, status code InternalServerError: invalid session "3103bd01-8165-4b14-cddd-542796ae036a"
  Stack Trace:
    at Consul.PutRequest`2[TIn,TOut].Execute (System.Threading.CancellationToken ct) [0x002c5] in <04779d9f65704d909393c78b5c007b81>:0 
  at Consul.Lock.Acquire (System.Threading.CancellationToken ct) [0x00447] in <04779d9f65704d909393c78b5c007b81>:0 
  at Consul.Lock.Acquire (System.Threading.CancellationToken ct) [0x00797] in <04779d9f65704d909393c78b5c007b81>:0 
  at Consul.Test.LockTest+<>c__DisplayClass9_1.<Lock_ContendFast>b__0 () [0x00096] in <3c10[17](https://github.com/G-Research/consuldotnet/runs/7343250916?check_suite_focus=true#step:9:18)3e055c4e98a841ccf76066[21](https://github.com/G-Research/consuldotnet/runs/7343250916?check_suite_focus=true#step:9:22)5d>:0 
  at Consul.Test.TimeoutUtils.WithTimeout (System.Threading.Tasks.Task task) [0x000fe] in <3c10173e055c4e98a841ccf76066215d>:0 
  at Consul.Test.LockTest.Lock_ContendFast () [0x000d8] in <3c10173e055c4e98a841ccf76066215d>:0

highlyunavailable and others added 7 commits July 21, 2022 19:52
# Conflicts:
#	Consul/Agent.cs
#	Consul/Client.cs
#	Consul/Client_Request.cs
#	Consul/Consul.csproj
#	Consul/Health.cs
#	Consul/KV.cs
#	Consul/Semaphore.cs
#	Consul/Utilities/JsonConverters.cs
this was needed by Newtonsoft.Json
nuget\packages\system.runtime.compilerservices.unsafe\6.0.0\buildTransitive\netcoreapp2.0\System.Runtime.CompilerServices.Unsafe.targets(4,5): error : System.Runtime.CompilerServices.Unsafe doesn't support netcoreapp2.1. Consider updating your TargetFramework to netcoreapp3.1 or later.
System.Text.Json 5.0.2 has a dependency on a version of System.Runtime.CompilerServices.Unsafe which still supports netcoreapp2.1
@mfkl
Copy link
Contributor Author

mfkl commented Aug 23, 2022

If we want to bump the minor version to 1.7 for this change, we may want to provide bindings for all 1.7 Consul APIs, such as Namespaces #117. This needs a consul enterprise version, so I reached out to Consul for it. Let's see.

@winstxnhdw
Copy link
Contributor

We can probably continue with this.

@marcin-krystianc
Copy link
Contributor

We can probably continue with this.

@winstxnhdw Feel free to give it a try.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is it possible to remove the dependecy on Newtonsoft.Json?
4 participants