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

Update to .NET 6.0 (updated NuGet packages/APIs) #13

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

Conversation

houstonhaynes
Copy link

This pull request is a "minimal" code change request - enough to simply get .NET6.0 and compatible libraries to compile/run.

@vany0114 vany0114 self-requested a review May 11, 2022 17:40
Copy link
Owner

@vany0114 vany0114 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @houstonhaynes this is great! just left a few comments/questions, looking forward to seeing this working with .Net 6, and thanks for taking the time to contribute to upgrading it!

BTW sorry about the delayed review.

@@ -14,7 +14,11 @@ by editing this MSBuild file. In order to learn more about this please visit htt
<SiteUrlToLaunchAfterPublish>https://paymentserviceexternal.azurewebsites.net</SiteUrlToLaunchAfterPublish>
<LaunchSiteAfterPublish>True</LaunchSiteAfterPublish>
<ExcludeApp_Data>False</ExcludeApp_Data>
<<<<<<< HEAD
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

heads up, looks like you forgot to resolve this conflict

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely didn't forget but maybe I forgot a push. I'll go back and take a look and button this up a bit more snugly. I am taking a Windows laptop with me on a working vacation next week so I hope to run "natively" on Service Mesh. I was locked in an AWS VDI at a client site and that hampered things a bit. Once I'm "on metal" I can submit something much more robust.


// Register all the event classes (they implement IAsyncNotificationHandler) in assembly holding the Commands
Autofac.Builder.IRegistrationBuilder<Mediator, Autofac.Builder.ConcreteReflectionActivatorData, Autofac.Builder.SingleRegistrationStyle> registrationBuilder2 = builder.RegisterType<Mediator>().As<IMediator>();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious if we could get rid of the variable since you're not using it

Suggested change
Autofac.Builder.IRegistrationBuilder<Mediator, Autofac.Builder.ConcreteReflectionActivatorData, Autofac.Builder.SingleRegistrationStyle> registrationBuilder2 = builder.RegisterType<Mediator>().As<IMediator>();
builder.RegisterType<Mediator>().As<IMediator>();

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was in "first do no harm" mode - and just wanted it to build/clear all warnings. That was in the info category and can certainly get rid of it.


builder.Register<SingleInstanceFactory>(context =>
Autofac.Builder.IRegistrationBuilder<ServiceFactory, Autofac.Builder.SimpleActivatorData, Autofac.Builder.SingleRegistrationStyle> registrationBuilder1 = builder.Register<ServiceFactory>(context =>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

});

builder.Register<MultiInstanceFactory>(context =>
Autofac.Builder.IRegistrationBuilder<ServiceFactory, Autofac.Builder.SimpleActivatorData, Autofac.Builder.SingleRegistrationStyle> registrationBuilder = builder.Register<ServiceFactory>(context =>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@@ -15,7 +15,7 @@

<ItemGroup>
<PackageReference Include="GeoCoordinate.NetCore" Version="1.0.0.1" />
<PackageReference Include="Kledex.Store.Cosmos.Mongo" Version="2.5.0" />
<PackageReference Include="OpenCqrs" Version="6.5.0" />
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't be OpenCqrs.Store.Cosmos.Mongo?

</PropertyGroup>

<ItemGroup>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious why you're including the OpenCqrs dependency in the shared kernel? 🤔

<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
</PackageReference>
<PackageReference Include="Microsoft.EntityFrameworkCore.SqlServer" Version="6.0.4" />
<PackageReference Include="OpenCqrs" Version="6.5.0" />
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto, why do we need OpenCqrs dependency here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I have been late getting back to this. OpenCqrs is the replacement for the earlier/deprecated version (Kledex) so I did a find and replace. If there's an entry here where it's not needed I'll nuke it.

</PackageReference>
<PackageReference Include="Microsoft.EntityFrameworkCore.SqlServer" Version="6.0.4" />
<PackageReference Include="OpenCqrs" Version="6.5.0" />
<PackageReference Include="System.Data.SqlClient" Version="4.8.3" />
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one too 🤔

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.

None yet

2 participants