Skip to content

Added support for .net 8 - DTF Service Fabric #1205

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jabhalla
Copy link

Added Support for .NET 8 - DTF Service Fabric Provider

Summary

This update introduces support for .NET 8 within the DTF Service Fabric Provider, alongside existing support for .NET Framework. Key changes and improvements are outlined below.

Changes

  1. Service Structure

    • Services are now created per target framework.
    • Each framework has a separate folder containing:
      • Controller
      • TaskHubProxyListener
      • Startup
  2. Framework-specific Integrations

    • .NET Framework continues to use OWIN.
    • .NET Core (including .NET 8) integrates with Kestrel.
  3. TaskHubProxyListenerBase

    • Introduced a new TaskHubProxyListenerBase class to abstract and centralize common functionality across frameworks.
  4. Endpoint Adjustments

    • In .NET Core, the same path cannot be used for two endpoints.
    • Created separate endpoints for:
      • orchestration
      • orchestrationAll
    • Updated RemoteOrchestrationServiceClient to align with these changes.

@jabhalla
Copy link
Author

@microsoft-github-policy-service agree company="Microsoft"

@jabhalla
Copy link
Author

jabhalla commented Apr 30, 2025

@cgillum / @jviau - Can you please add @shankarsama as reviewer? I would appreciate a review from one of you gus as well. Also, can you please approve the ci pipeline?

Thanks

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a bunch of package changes here that will impact all projects in this repo. Have you verified what the impact to other packages are? Keep in mind we cannot have any major version revs of any transitive dependency. This is considered a breaking change to customers and we would need to also major version rev.

Copy link
Author

Choose a reason for hiding this comment

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

None of the other project's target .net8. All projects continue to be on the same version. Updated the Packages.props to a more understandable structure.

<PropertyGroup>
<TargetFrameworks>net462;net472</TargetFrameworks>
<TargetFrameworks>net8.0;net462;net472</TargetFrameworks>
Copy link
Collaborator

Choose a reason for hiding this comment

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

These changes look fairly extensive, especially adding a new TFM. Have you evaluated what version change needs to happen? if there are any breaking changes (even revving transitive dependencies across major versions), then this package will need a major version rev as well.

@jviau
Copy link
Collaborator

jviau commented Apr 30, 2025

These changes are very large and impactful. I think it will need to go through a breaking change review and potential major version rev.

@jviau jviau self-requested a review May 12, 2025 16:36
<PackageVersion Include="Microsoft.Extensions.DependencyInjection" Version="2.1.1" Condition="'$(TargetFramework)' != 'net8.0'" />
<PackageVersion Include="Microsoft.Extensions.Logging.Abstractions" Version="6.0.1" Condition="'$(TargetFramework)' != 'net8.0'" />
<PackageVersion Include="Microsoft.Extensions.Logging" Version="6.0.0" Condition="'$(TargetFramework)' != 'net8.0'" />
<PackageVersion Include="Microsoft.Extensions.Logging.Console" Version="6.0.0" Condition="'$(TargetFramework)' != 'net8.0'" />
Copy link
Member

Choose a reason for hiding this comment

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

I'm worried about all these != 'net8.0' checks, and how we will maintain these if/when we need to move to net10 in the future. Is there another way we can refactor this to make it easier to make future changes?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

@jviau - Can you please elaborate? I din't follow. Do you mean using IsTargetFrameworkCompatible?

Condition="!$([MSBuild]::IsTargetFrameworkCompatible('net8.0', '$(TargetFramework)'))"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes IsTargetFrameworkCompatible might be what you want. But I can't remember the exact behavior/syntax off the top of my head. But you should be able to use it to effectively do a >= net8.0

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.

3 participants