modernize ASP.NET Core samples#326
Conversation
|
Just to set expectations, I'll look at this when I can, but it's unlikely to be for at least a week. |
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="Microsoft.AspNetCore.Mvc.Testing" /> | ||
| <PackageReference Include="Microsoft.AspNetCore.Mvc.Testing" VersionOverride="10.0.5" /> <!-- VersionOverride is needed to avoid pulling in a later version of Microsoft.AspNetCore.App that isn't compatible with .NET 10.0. --> |
There was a problem hiding this comment.
I could convert the whole repo into .NET 10 in a successive PR, so we don't suffer from tricks like this. I'm not sure how much impact that has yet, so I refrained from making this current change too big.
There was a problem hiding this comment.
We still want to support .NET 8 while that's a supported platform. We may be able to target net8 while using the .NET 10 SDK, but I definitely don't want to remote a net8 target.
There was a problem hiding this comment.
You'd like to keep net8 on the IntegrationTests and AspNetCoreSample projects as well?
There was a problem hiding this comment.
Not sure, and I don't have the bandwidth to think about it right now. Leave this comment thread open, and I'll reply again when I get round to reviewing the whole PR.
| Assert.Contains(cloudEvent.Id, resultContent); | ||
| Assert.Contains(cloudEvent.Type, resultContent); | ||
| Assert.Contains($"\"{expectedExtensionKey}\": \"{expectedExtensionValue}\"", resultContent); | ||
| Assert.Contains($"\"{expectedExtensionKey}\":\"{expectedExtensionValue}\"", resultContent); |
There was a problem hiding this comment.
As we notice here, the Json serializer is handling this slightly different, I think because we're using System.Text.Json now throughout the ASP.NET Core sample. In a successive PR, i could make this a bit more robust.
|
Thanks for your quick heads-up @jskeet. I think the PR is ready to go and I'm happy to continue on some other maintenance work related to this repo, in successive PRs. |
|
|
||
| namespace CloudNative.CloudEvents.AspNetCoreSample | ||
| { | ||
| // FIXME: This doesn't get called for binary CloudEvents without content, or with a different data content type. |
There was a problem hiding this comment.
The FIXME's are still relevant, I guess. I'd rather put those in separate issues in the GitHub repo so it can be picked up from there.
erwinkramer
force-pushed
the
main
branch
2 times, most recently
from
42977d0 to
0b7f66b
Compare
Signed-off-by: Erwin <erwinkramer@hotmail.com>
Signed-off-by: Erwin <erwinkramer@hotmail.com>
Signed-off-by: Erwin <erwinkramer@hotmail.com>
Signed-off-by: Erwin <erwinkramer@hotmail.com>
Signed-off-by: Erwin <erwinkramer@hotmail.com>
Signed-off-by: Erwin <erwinkramer@hotmail.com>
Signed-off-by: Erwin <erwinkramer@hotmail.com>
Signed-off-by: Erwin <erwinkramer@hotmail.com>
Signed-off-by: Erwin <erwinkramer@hotmail.com>
Signed-off-by: Erwin <erwinkramer@hotmail.com>
Signed-off-by: Erwin <erwinkramer@hotmail.com>
2 participants
This modernizes the ASP.NET Core samples in a couple of ways:
slnxinstead ofslnsolutionIBindableFromHttpContextto bind an incoming CloudEvent, instead of controller basedMicrosoft.AspNetCore.Mvc.FormattersSystemTextJsoninstead ofNewtonsoftJsonGenerateCloudEventmethod by using the nativeCopyToHttpResponseAsyncfromCloudNative.CloudEvents.AspNetCoreSuccessful build @ https://github.com/erwinkramer/sdk-csharp/actions/runs/23054867045