-
-
Notifications
You must be signed in to change notification settings - Fork 210
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 Sentry.Tunnel to add official tunnel middleware #1133
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this again. THere are some small things before we can merge, could you please take a look?
Also, we could just target netcoreapp3.1 and not bother with 2.1 which is about to run out of support and this is anyway a new feature so requiring 3.1 isn't anything crazy.
<Project Sdk="Microsoft.NET.Sdk"> | ||
|
||
<PropertyGroup> | ||
<TargetFrameworks>net5.0;netcoreapp3.0;netstandard2.0</TargetFrameworks> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about just netcoreapp3.1
and netstandard2.0
?
<TargetFrameworks>net5.0;netcoreapp3.0;netstandard2.0</TargetFrameworks> | |
<TargetFrameworks>netcoreapp3.1;netstandard2.0</TargetFrameworks> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never bothered with 2.1, only 3.0, not sure if there is a point to do 3.1 instead. I added net5.0 just to ensure we have a version that binds against ASP.NET Core 5, just in case. Not sure if we need it but gotta make sure it doesn't pull in weird, old dependencies.
src/Sentry.Tunnel/SentryTunnelingApplicationBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
<Project Sdk="Microsoft.NET.Sdk"> | ||
|
||
<PropertyGroup> | ||
<TargetFrameworks>net5.0;netcoreapp3.1;netcoreapp2.1</TargetFrameworks> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case you take the change for the library:
<TargetFrameworks>net5.0;netcoreapp3.1;netcoreapp2.1</TargetFrameworks> | |
<TargetFrameworks>netcoreapp3.1;netcoreapp2.1</TargetFrameworks> |
<ItemGroup> | ||
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.9.4" /> | ||
<PackageReference Include="coverlet.collector" Version="3.0.2"> | ||
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets> | ||
<PrivateAssets>all</PrivateAssets> | ||
</PackageReference> | ||
</ItemGroup> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is declared on the parent folder's Directory.Build.props
<ItemGroup> | |
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.9.4" /> | |
<PackageReference Include="coverlet.collector" Version="3.0.2"> | |
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets> | |
<PrivateAssets>all</PrivateAssets> | |
</PackageReference> | |
</ItemGroup> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are needed because the test project doesn't inherit Sentry.Testing, and if I inherit from that it fails to build...
<!-- Run netcoreapp3.0 against netstandard2.1 target of Sentry --> | ||
<ItemGroup Condition="'$(TargetFramework)' == 'netcoreapp3.0'"> | ||
<ProjectReference Update="../../src/Sentry/Sentry.csproj" SetTargetFramework="TargetFramework=netstandard2.1" /> | ||
<!-- NET Core's built-in STJ is lower version which causes conflicts, so we have to explicitly reference it --> | ||
<PackageReference Include="System.Text.Json" Version="5.0.2" /> | ||
<!-- Ben.Demystifier uses S.R.M v5 and also requires it via package reference when on nca3.x --> | ||
<PackageReference Include="System.Reflection.Metadata" Version="5.0.0" /> | ||
</ItemGroup> | ||
|
||
<ItemGroup Condition="$(TargetFramework) == 'netcoreapp2.1' OR $(TargetFramework) == 'net461'"> | ||
<PackageReference Include="Microsoft.AspNetCore.TestHost" Version="2.1.1" /> | ||
</ItemGroup> | ||
|
||
<ItemGroup Condition="$(TargetFramework) == 'netcoreapp3.1'"> | ||
<PackageReference Include="Microsoft.AspNetCore.TestHost" Version="3.1.0" /> | ||
</ItemGroup> | ||
|
||
<ItemGroup Condition="$(TargetFramework) == 'net5.0'"> | ||
<PackageReference Include="Microsoft.AspNetCore.TestHost" Version="5.0.0" /> | ||
</ItemGroup> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need any of this?
<!-- Run netcoreapp3.0 against netstandard2.1 target of Sentry --> | |
<ItemGroup Condition="'$(TargetFramework)' == 'netcoreapp3.0'"> | |
<ProjectReference Update="../../src/Sentry/Sentry.csproj" SetTargetFramework="TargetFramework=netstandard2.1" /> | |
<!-- NET Core's built-in STJ is lower version which causes conflicts, so we have to explicitly reference it --> | |
<PackageReference Include="System.Text.Json" Version="5.0.2" /> | |
<!-- Ben.Demystifier uses S.R.M v5 and also requires it via package reference when on nca3.x --> | |
<PackageReference Include="System.Reflection.Metadata" Version="5.0.0" /> | |
</ItemGroup> | |
<ItemGroup Condition="$(TargetFramework) == 'netcoreapp2.1' OR $(TargetFramework) == 'net461'"> | |
<PackageReference Include="Microsoft.AspNetCore.TestHost" Version="2.1.1" /> | |
</ItemGroup> | |
<ItemGroup Condition="$(TargetFramework) == 'netcoreapp3.1'"> | |
<PackageReference Include="Microsoft.AspNetCore.TestHost" Version="3.1.0" /> | |
</ItemGroup> | |
<ItemGroup Condition="$(TargetFramework) == 'net5.0'"> | |
<PackageReference Include="Microsoft.AspNetCore.TestHost" Version="5.0.0" /> | |
</ItemGroup> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need it because the test project doesn't reference Sentry.Testing or whatever
Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>
Co-authored-by: LucasZF <lucas-zimerman1@hotmail.com>
Co-authored-by: LucasZF <lucas-zimerman1@hotmail.com>
Co-authored-by: LucasZF <lucas-zimerman1@hotmail.com>
Co-authored-by: LucasZF <lucas-zimerman1@hotmail.com>
Co-authored-by: LucasZF <lucas-zimerman1@hotmail.com>
Co-authored-by: LucasZF <lucas-zimerman1@hotmail.com>
Co-authored-by: LucasZF <lucas-zimerman1@hotmail.com>
Co-authored-by: LucasZF <lucas-zimerman1@hotmail.com>
Co-authored-by: LucasZF <lucas-zimerman1@hotmail.com>
Co-authored-by: LucasZF <lucas-zimerman1@hotmail.com>
Visual Studio is nagging me about
Naming rule violation: Missing suffix: 'Async'
on the tests, no clue why. It works otherwise.#skip-changelog.