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.
There was a problem hiding this comment.
Okay, I've thought about it more - I'm fine with this being net10.0 only, and likewise IntegrationTests. I want to keep the production code projects targeting net8.0 for now. We can add a target for net10.0 (in a separate PR) so that we can remove net8.0 when it goes EOL.
There was a problem hiding this comment.
The VersionOverride is still in there for now. If I'd have to remove it (and bump the directory package version to 10), I have to modify the GitHub workflow to only run tests on .NET 10, right? Not sure if you want that at this stage.
test/CloudNative.CloudEvents.IntegrationTests/AspNetCore/CloudEventBindingTests.cs
Show resolved
Hide resolved
|
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. |
samples/CloudNative.CloudEvents.AspNetCoreSample/CloudEventJsonInputFormatter.cs
Show resolved
Hide resolved
2ca5aaa to
5cccd96
Compare
| using System.Collections.Generic; | ||
| using System.Threading.Tasks; | ||
|
|
||
| namespace CloudNative.CloudEvents.AspNetCoreSample.Operations |
There was a problem hiding this comment.
Let's use a file-scoped namespace for new code.
There was a problem hiding this comment.
And if this is in a different namespace, please move it to the corresponding folder. (Does it need to be in a different namespace?)
There was a problem hiding this comment.
Changed to file-scoped for the samples (including HttpSend sample). For the namespace, I'm not sure if it's worth it to keep a distinct namespace. For simplicity's sake I changed the whole project as CloudNative.CloudEvents.AspNetCoreSample namespace for now, otherwise binding and operations files would each have their own namespace and folder, which is maybe a little too much for those 2 files.
samples/CloudNative.CloudEvents.AspNetCoreSample/CloudEventOperations.cs
Outdated
Show resolved
Hide resolved
global.json
Outdated
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "sdk": { | |||
| "version": "8.0.400", | |||
There was a problem hiding this comment.
Adding this comment here, but basically I'd quite like to keep all the "general infrastructure update" changes separate from the sample changes. If I create a PR that makes these changes separately, are you happy to rebase on top of that? (The PR itself will be simple, but I'll need to get it reviewed of course.)
There was a problem hiding this comment.
I guess that would include the sln to slnx migration from this PR as well?
There was a problem hiding this comment.
See #327 - I'm hoping I can get that reviewed and merged this week.
test/CloudNative.CloudEvents.IntegrationTests/AspNetCore/CloudEventBindingTests.cs
Show resolved
Hide resolved
samples/CloudNative.CloudEvents.AspNetCoreSample/CloudEventJsonInputFormatter.cs
Show resolved
Hide resolved
|
|
||
| public ProblemHttpResult Error { get; init; } | ||
|
|
||
| public static async ValueTask<CloudEventBinding> BindAsync(HttpContext context, ParameterInfo parameter) |
There was a problem hiding this comment.
Just as a heads-up, I'm really not familiar with IBindableFromHttpContext. I'm mostly following along, but I could easily miss things.
|
|
||
| <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.
Okay, I've thought about it more - I'm fine with this being net10.0 only, and likewise IntegrationTests. I want to keep the production code projects targeting net8.0 for now. We can add a target for net10.0 (in a separate PR) so that we can remove net8.0 when it goes EOL.
|
Infrastructure PR now merged, so you should be able to rebase onto it. |
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>
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>
- Convert .sln to .slnx - Update to .NET 10 SDK - Update unit test targets to net8.0 and net10.0 - Update integration test target to net10.0 - Update sample targets to net10.0 - Update all dependencies as far as they're compatible - Update protobuf generator and regenerate - Update actions/setup-dotnet workflow action Signed-off-by: Jon Skeet <skeet@pobox.com> Signed-off-by: Erwin <erwinkramer@hotmail.com>
Signed-off-by: Erwin <erwinkramer@hotmail.com>
This reverts commit f305671. Signed-off-by: Erwin <erwinkramer@hotmail.com>
|
@jskeet I guess it's ready to be merged now. |
|
I'll have another look when I can - maybe tomorrow evening, but it may be Friday. |
Signed-off-by: Erwin <erwinkramer@hotmail.com>
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