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 Sentry.Tunnel to add official tunnel middleware #1133

Merged
merged 16 commits into from
Jul 25, 2021

Conversation

kanadaj
Copy link
Contributor

@kanadaj kanadaj commented Jul 18, 2021

Visual Studio is nagging me about Naming rule violation: Missing suffix: 'Async' on the tests, no clue why. It works otherwise.

#skip-changelog.

Copy link
Member

@bruno-garcia bruno-garcia left a 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>
Copy link
Member

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?

Suggested change
<TargetFrameworks>net5.0;netcoreapp3.0;netstandard2.0</TargetFrameworks>
<TargetFrameworks>netcoreapp3.1;netstandard2.0</TargetFrameworks>

Copy link
Contributor Author

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/Sentry.Tunnel.csproj Show resolved Hide resolved
src/Sentry.Tunnel/Sentry.Tunnel.csproj.DotSettings Outdated Show resolved Hide resolved
src/Sentry.Tunnel/SentryTunnelMiddleware.cs Show resolved Hide resolved
src/Sentry.Tunnel/SentryTunnelMiddleware.cs Outdated Show resolved Hide resolved
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFrameworks>net5.0;netcoreapp3.1;netcoreapp2.1</TargetFrameworks>
Copy link
Member

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:

Suggested change
<TargetFrameworks>net5.0;netcoreapp3.1;netcoreapp2.1</TargetFrameworks>
<TargetFrameworks>netcoreapp3.1;netcoreapp2.1</TargetFrameworks>

Comment on lines +8 to +15
<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>

Copy link
Member

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

Suggested change
<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>

Copy link
Contributor Author

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

test/Sentry.Tunnel.Tests/Sentry.Tunnel.Tests.csproj Outdated Show resolved Hide resolved
Comment on lines 22 to 41
<!-- 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>
Copy link
Member

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?

Suggested change
<!-- 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>

Copy link
Contributor Author

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

kanadaj and others added 7 commits July 23, 2021 21:53
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>
kanadaj and others added 3 commits July 23, 2021 21:54
Co-authored-by: LucasZF <lucas-zimerman1@hotmail.com>
Co-authored-by: LucasZF <lucas-zimerman1@hotmail.com>
Co-authored-by: LucasZF <lucas-zimerman1@hotmail.com>
@bruno-garcia bruno-garcia merged commit cd03fe1 into getsentry:main Jul 25, 2021
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.

None yet

3 participants