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

Add MSBuild SDK #288

Merged
merged 3 commits into from
Dec 10, 2023
Merged

Add MSBuild SDK #288

merged 3 commits into from
Dec 10, 2023

Conversation

Xeinaemm
Copy link
Contributor

@Xeinaemm Xeinaemm commented Dec 9, 2023

The goal is to clean up and standardize projects.

Changes:

  • Added Directory.Build.props and Directory.Build.targets to define a standardized approach for all projects.
  • Removed property CopyToOutputDirectory="Always" for appsetings.json files because it prevents reuse of build artifacts. For clean builds should be used clean functionality.
  • Added implicit usings.
  • (CHANGE REMOVED) Added AOT publish to get faster startup and better optimization of application but with the cost of artifact build and publish time.
  • Enabled analyzers and StyleCop with respective configuration files for all projects.
  • 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.
  • Updated database to .NET Framework 4.8

@bistok
Copy link
Contributor

bistok commented Dec 9, 2023

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
Copy link
Owner

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 :)

Copy link
Owner

@kgrzybek kgrzybek left a comment

Choose a reason for hiding this comment

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

  1. Remove AOT
  2. Revert StyleCop downgrade

@Xeinaemm
Copy link
Contributor Author

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
Copy link
Contributor Author

Xeinaemm commented Dec 10, 2023

To consider:

  • Move cross-module IntegrationEvents into MSBuild SDK. It will standardize all types of references but will result in having all such event references per module. For 4 modules it makes sense but for 100 it will result in 99 references.
  • 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).

@AlmarAubel
Copy link
Contributor

AlmarAubel commented Dec 10, 2023

  • 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
Copy link
Contributor Author

Xeinaemm commented Dec 10, 2023

@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
Copy link
Contributor

bistok commented Dec 10, 2023

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 #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.

@bistok bistok mentioned this pull request Dec 10, 2023
@Xeinaemm
Copy link
Contributor Author

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
Copy link
Contributor

bistok commented Dec 10, 2023

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 kgrzybek merged commit 5eb11ed into kgrzybek:master Dec 10, 2023
2 checks passed
@kgrzybek
Copy link
Owner

Merged, thanks.

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

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.

4 participants