search

kgrzybek / modular-monolith-with-ddd

Full Modular Monolith application with Domain-Driven Design approach.
MIT License
10.82k stars 1.7k forks source link

Add MSBuild SDK #288

Closed Xeinaemm closed 8 months ago

Xeinaemm commented 8 months ago

The goal is to clean up and standardize projects.

Changes:

bistok commented 8 months ago

I think that the PublishAot it not a good option because in some parts the system uses Reflection and produces many warning when publishing The other thing is StyleCop.Analyzers 1.1 target C# 7 and 1.2 Targets C#8, I think it's better to minimum target C#8

kgrzybek commented 8 months ago

Hi @Xeinaemm, firstly, thanks for your great work! :)

Added Directory.Build.props and Directory.Build.targets to define a standardized approach for all projects.

Yes, this is good approach to have things standardized. OK

Removed property CopyToOutputDirectory="Always" for appsetings.json files because it prevents reuse of build artifacts. For clean builds should be used clean functionality.

OK

Added implicit usings.

OK

Added AOT publish to get faster startup and better optimization of application but with the cost of artifact build and publish time.

As @bistok said, there are a lot of warnings right now and I think it is not need. Moreover, it adds more risk that on runtime something can blow ;) Revert this change please :)

Enabled analyzers and StyleCop with respective configuration files for all projects.

It is good, but I agree with @bistok so please use 1.2.0-beta.507

Removed project BuildingBlocks.EventBus and moved it into BuildingBlocks.Infrastructure. The latter should be superior to the rest foundation libraries. EventBus makes sense only if the whole functionality will be migrated to a separate library. Otherwise, it creates very heavy dependency(infrastructure) and a lot of problems with circular dependencies.

It make sense for now. OK

Updated database to .NET Framework 4.8

OK

And general comment - please add separate PR's for different improvements or just separate commits - it would be easier to review it :)

Xeinaemm commented 8 months ago

I will check it. Under the hood, there is also an issue with IdentityServer4, which is not compatible with anything above .NET Core 3.1. Normally I would suggest upgrading to Duende IdentityServer v7, but this is a paid version.

Xeinaemm commented 8 months ago

To consider:

AlmarAubel commented 8 months ago
  • Standardize test NuGet packages by having one base project for all of them which injects all necessary dependencies and adds it into MSBuild SDK. On adding a new test library it will inject all necessary dependencies. The second option is to use pure MSBuild SDK but it will result in an inability to use the UI NuGet manager. All references put in MSBuild must be updated manually or by script. Otherwise, UI will create changes in all projects rather than in one place(MSBuild SDK).

Another options is to use Central package management. Which is supported by VS and Rider. There is also a tool to convert this whole project to central package management see this Github repo.

There still will be option to override package versions on project level if needed.

In the Directory.build.props file you can add something like this:

  <ItemGroup Condition="$(MSBuildProjectName.EndsWith('Tests'))">
        <Using Include="FluentAssertions" />        
        <Using Include="Xunit" />
        <Using Include="Xunit.Abstractions" />

       <PackageReference Include="FluentAssertions" />
      <PackageReference Include="xunit" />
      ... other packages
    </ItemGroup>
Xeinaemm commented 8 months ago

@AlmarAubel It works like a charm!

  1. Define Directory.Packages.props
  2. Put all references with conditions into it.

There is no need for enabled CPM.

Ref: https://github.com/Xeinaemm/modular-monolith-with-ddd/commit/25c84f0145531ad98044026891a57c79c337f708

bistok commented 8 months ago

I will check it. Under the hood, there is also an issue with IdentityServer4, which is not compatible with anything above .NET Core 3.1. Normally I would suggest upgrading to Duende IdentityServer v7, but this is a paid version.

I have done some checks and updated the code with a PR that is merged right now https://github.com/kgrzybek/modular-monolith-with-ddd/pull/287

I ran some test using all the API get methods with authentication and everything executed ok

I think we can change the Auth to be using ASP.NET 8 Identity with tokens. I can explore this path.

Xeinaemm commented 8 months ago

I have done some checks and updated the code with a PR that is merged right now #287

I ran some test using all the API get methods with authentication and everything executed ok

I think we can change the Auth to be using ASP.NET 8 Identity with tokens. I can explore this path.

I'm after migration from IdentityServer4 to Duende IdentityServer with 17 million users. Most critical products will not accept such a risk of not having the latest updates or an up-to-date SLA.

It's good to look at ASP.NET 8 Identity to reduce costs, but beyond that, we should promote the move to Duende IdentityServer.

bistok commented 8 months ago

I have done some checks and updated the code with a PR that is merged right now #287 I ran some test using all the API get methods with authentication and everything executed ok I think we can change the Auth to be using ASP.NET 8 Identity with tokens. I can explore this path.

I'm after migration from IdentityServer4 to Duende IdentityServer with 17 million users. Most critical products will not accept such a risk of not having the latest updates or an up-to-date SLA.

It's good to look at ASP.NET 8 Identity to reduce costs, but beyond that, we should promote the move to Duende IdentityServer.

Let’s discuss this outside this PR on the thread I open #289 . To have this only related to the PR

kgrzybek commented 8 months ago

Merged, thanks.

Let's discuss authentication implementation and global packages management in other issues.