Upgraded Newtonsoft and added ServiceBus Changes for MSI Auth on NET …

[namanjainv]

…461 and greater

[cgillum]

Hi @namanjainv, can you reduce the scope of this PR to only touch Service Bus-related code? We can't accept the changes for DurableTask.AzureStorage, for example, because those will break existing customers in Azure Functions. We have fewer restrictions on the Service Bus-related projects.

[cgillum]

Missing from this PR is a change to the package version. Normally for this package it comes from /tools/DurableTask.props. However, we're trying to instead keep version numbers in the package that we're actually versioning rather than having a common version configuration. Can you add the following markup to DurableTask.ServiceBus.csproj?

  <!-- Version Info -->
  <PropertyGroup>
    <MajorVersion>2</MajorVersion>
    <MinorVersion>7</MinorVersion>
    <PatchVersion>0</PatchVersion>

    <VersionPrefix>$(MajorVersion).$(MinorVersion).$(PatchVersion)</VersionPrefix>
    <FileVersion>$(VersionPrefix).0</FileVersion>
    <!-- FileVersionRevision is expected to be set by the CI. This is useful for distinguishing between multiple builds of the same version. -->
    <FileVersion Condition="'$(FileVersionRevision)' != ''">$(VersionPrefix).$(FileVersionRevision)</FileVersion>
    <!-- The assembly version is only the major/minor pair, making it easier to do in-place upgrades -->
    <AssemblyVersion>$(MajorVersion).$(MinorVersion).0.0</AssemblyVersion>
    <!-- This version is used as the nuget package version -->
    <Version>$(VersionPrefix)</Version>
  </PropertyGroup>

Note that I suggested this to be v2.7.0, but we may want to consider it to be 3.0.0 since we're making potentially breaking changes by upgrading dependencies. See some of my other comments too, which might impact the final version we go with.

[cgillum]

With your latest changes, is it possible to only target netstandard2.0 instead of both net462 and netstandard2.0? That would be preferred since it will reduce the size of the package.

[namanjainv]

I tried to remove net462 dependencies, but this started throwing multiple build-time errors. For example System.Transaction was not available. And I dont think we need to remove net462 from TargetFrameworks right?

Incase I remove, do I need to remove net462 test cases too?

[cgillum]

/azp run

[azure-pipelines]

Azure Pipelines started successfully

[cgillum]

I'm good with these changes now, and the CI is passing.

[cgillum]

Actually, it looks like the CI didn't run any of the Service Bus tests due to a filter which is no longer finding them. I'll need to update the CI to account for this.

[namanjainv]

Actually, it looks like the CI didn't run any of the Service Bus tests due to a filter which is no longer finding them. I'll need to update the CI to account for this.

Sounds good

[cgillum]

I assume this was closed accidentally?

[namanjainv]

Yup. My bad

[jviau]

@namanjainv to make sure I understand this change correctly: it looks like you are removing the net461 TFM and all of its ifdef logic. Is the end result that the MSI features you want were behind netstandard2.0 ifdefs and thus not available when targeting netfx?

If the above is correct, this is really interesting that you did not have to add any code, just remove that net461 specifics. I am curious if there was a reason this was never added to net461 in the first place? @cgillum do you know why?

[cgillum]

@jviau my memory is a bit fuzzy, but I think the main reason we originally had multiple TFMs was because DT.Core, DT.ServiceBus, and some other dependencies were targeting net451, which was not compatible with netstandard2.0. Since then, the older TFMs have been replaced with newer ones, but I think we simply never got around to just collapsing everything to netstandard2.0, like what is being done in this PR.

[cgillum]

This change requires us to update our CI pipeline because the location of the test DLLs will change. Unfortunately, this CI was created before it was possible to use YAML files in the repo (and we haven't converted it to YAML yet), so we have to use the ADO UX to make these updates.

@namanjainv can you tell me where I'll be able to find the Service Bus test DLLs in the build output (paths relative to the repo root)?

[namanjainv]

@cgillum, The DLL is generated at "Test\DurableTask.ServiceBus.Tests\bin\Debug\netcoreapp3.1\DurableTask.ServiceBus.dll"

[cgillum]

/azp run

[azure-pipelines]

EDIT: Test run triggered successfully.