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 netstandard2.0 target for ServerTelemetryChannel and remove Newtonsoft.Json dependency (#800) #801

Merged

Conversation

clguiman
Copy link
Member

Add netstandard2.0 target for ServerTelemetryChannel and remove Newtonsoft.Json dependency (#800)

Fix Issue #800 .
Replaced JavaScriptSerializer with DataContractjsonSerializer (exists in netstandard 2.0 & net45)
I had to update the BackendResponse member attributes with IsRequired=true so BackoffLogicManagerTest.IfContentIsUnexpectedJsonNullIsReturned won't fail.

For significant contributions please make sure you have completed the following items:

  • Design discussion issue #
  • Changes in public surface reviewed
  • CHANGELOG.md updated with one line description of the fix, and a link to the original issue.
  • The PR will trigger build, unit test, and functional tests automatically. If your PR was submitted from fork - mention one of committers to initiate the build for you.

@cijothomas
Copy link
Contributor

@clguiman clguiman force-pushed the clguiman/telemetrychannel-netstandard2.0 branch from 622d0a3 to 9441010 Compare May 14, 2018 21:30
@clguiman
Copy link
Member Author

@cijothomas I've added the 2.0 tests

@clguiman clguiman force-pushed the clguiman/telemetrychannel-netstandard2.0 branch 9 times, most recently from ab61b54 to 51feb6f Compare May 15, 2018 17:48
@TimothyMothra
Copy link
Member

@clguimanMSFT Can you please update the changelog?

@clguiman
Copy link
Member Author

@MS-TimothyMothra should I add the changes under 2.7.0-beta1?

@TimothyMothra
Copy link
Member

@clguimanMSFT you can call it 2.6.2.
we're going to get this out right away.

Common.props Outdated
@@ -21,7 +21,7 @@
<CodeAnalysisRuleSet>$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildProjectDirectory), 'ApplicationInsightsSDKRules.ruleset'))\ApplicationInsightsSDKRules.ruleset</CodeAnalysisRuleSet>
</PropertyGroup>

<PropertyGroup Condition="'$(Configuration)' == 'Release'">
<PropertyGroup Condition="'$(Configuration)' == 'Release' and '$(TargetFramework)'!='netstandard2.0'">
Copy link
Contributor

Choose a reason for hiding this comment

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

#803 - so that we know this is disabled for 2.0 target

Copy link
Member

Choose a reason for hiding this comment

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

The condition should be on the "RunCodeAnalysis" property. The other properties (particularly Optimize and DefineConstants) should still be set.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed it

@cijothomas
Copy link
Contributor

@SergeyKanzhelev / @Dmitry-Matveev - Please have a look at this. We have modified build defs to test this (and manually ran them - all good)

@clguiman clguiman force-pushed the clguiman/telemetrychannel-netstandard2.0 branch from 9cf4d02 to 606845f Compare May 15, 2018 19:30
<DefineConstants>$(DefineConstants);NETCOREAPP</DefineConstants>
</PropertyGroup>

<ItemGroup>
Copy link
Member

Choose a reason for hiding this comment

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

Should we add conflicting Newtonsoft for the test app to make sure our dependency does not get back with the future commits? (and if it does, the test will fail?)

@cijothomas cijothomas merged commit 90623fb into microsoft:develop May 15, 2018
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.

5 participants