ASPNET.Core default Environment settings are cleaned, if used. #554

PureKrome · 2020-10-18T13:49:46Z

Resolves #551

NOTES:

The ASPNET.Core project actually targets both NETCOREAPP30 and NS20 (I'm not sure why .....).

<TargetFrameworks>netcoreapp3.0;netstandard2.0</TargetFrameworks>

As such, I've ended up NOT doing a single "if environment == "Production" string check (for both NS20 and NCA30) because NETCOREAPP30+ now includes this string as a constant. Yay 🎉

So I ended up using #if NETSTANDARD2_0 etc, preprocessor directives. It makes the code a bit more messy, so i'm totally happy to strip that out and just have the a simple string check for both, instead of a string check and a const string check.

Finally, not sure if the test I added was into the correct file, etc. So happy to get told otherwise.

PureKrome · 2020-10-18T14:09:16Z

@bruno-garcia Drats - the test failed, yet it works on my machine. Is there any way I can see what the failure is?

(BTW, I can't run any NET461 or NC21 tests on my machine. I only have NetCore3+ installed)

Not sure if the failing test occurs there/then?

src/Sentry.AspNetCore/SentryAspNetCoreOptionsSetup.cs

Co-authored-by: Brian Surowiec <[email protected]>

codecov-io · 2020-10-18T22:38:38Z

Codecov Report

Merging #554 into main will decrease coverage by 0.09%.
The diff coverage is 86.25%.

@@            Coverage Diff             @@
##             main     #554      +/-   ##
==========================================
- Coverage   85.33%   85.23%   -0.10%     
==========================================
  Files         128      138      +10     
  Lines        3170     3332     +162     
  Branches      730      752      +22     
==========================================
+ Hits         2705     2840     +135     
- Misses        268      294      +26     
- Partials      197      198       +1

Impacted Files	Coverage Δ
src/Sentry/Protocol/BaseScopeExtensions.cs	`86.66% <ø> (ø)`
src/Sentry/Protocol/Context/Contexts.cs	`90.90% <ø> (ø)`
src/Sentry/Protocol/Request.cs	`92.00% <ø> (ø)`
src/Sentry/Internal/Polyfills.cs	`50.00% <50.00%> (ø)`
src/Sentry/Protocol/EnvelopeItem.cs	`60.00% <60.00%> (ø)`
src/Sentry/Protocol/StreamSerializable.cs	`66.66% <66.66%> (ø)`
src/Sentry/SentryClient.cs	`80.45% <66.66%> (-2.10%)`	⬇️
src/Sentry/Internal/Http/HttpTransport.cs	`89.47% <88.88%> (+2.80%)`	⬆️
src/Sentry/Internal/BackgroundWorker.cs	`84.28% <93.18%> (+0.82%)`	⬆️
src/Sentry/Protocol/Envelope.cs	`96.15% <96.15%> (ø)`
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update efeab33...890c095. Read the comment docs.

test/Sentry.AspNetCore.Tests/SentryAspNetCoreOptionsSetupTests.cs

Co-authored-by: Brian Surowiec <[email protected]>

test/Sentry.AspNetCore.Tests/SentryAspNetCoreOptionsSetupTests.cs

PureKrome · 2020-10-19T04:10:11Z

@bruno-garcia as a side note to this PR, i don't understand how my PR dropped that files code coverage by 8% 😢

I'd love to know how/why so I can make sure I'm covering the code I just created.

(note: I did click on the file in the codecoverage report above and not sure how to make it out :/ )

Tyrrrz · 2020-10-19T15:10:32Z

The ASPNET.Core project actually targets both NETCOREAPP30 and NS20 (I'm not sure why .....).

That's mainly because NS20 doesn't have nullable type annotations on BCL. NS20 is still required for older versions of the framework, I'm guessing.

i don't understand how my PR dropped that files code coverage by 8%

That was probably outdated, shows +0.04% now.

src/Sentry.AspNetCore/SentryAspNetCoreOptionsSetup.cs

Tyrrrz · 2020-10-19T15:18:31Z

src/Sentry.AspNetCore/SentryAspNetCoreOptionsSetup.cs

+                    //             need to respect (especially the case-sensitivity).
+                    //             REF: https://docs.microsoft.com/en-us/aspnet/core/fundamentals/environments
+#if NETSTANDARD2_0
+                    if (_hostingEnvironment.EnvironmentName.Equals("Production"))


Should we consider case sensitivity here? Or maybe use StringComparison.OrdinalIgnoreCase?

re: exact casing:

I did case sensitivity to reduce cycles. Why set the value if it's already the exact same?

EDIT: Also not sure if there's an extra allocation if setting the string value, if the previous string value is already the same.

src/Sentry.AspNetCore/SentryAspNetCoreOptionsSetup.cs

bruno-garcia

great stuff!

src/Sentry.AspNetCore/SentryAspNetCoreOptionsSetup.cs

Co-authored-by: Bruno Garcia <[email protected]>

PureKrome · 2020-10-20T00:27:32Z

sadness 😢 AppVeyor is dying cause of some transient error :(

  Determining projects to restore...
C:\projects\sentry-dotnet\.tmp\dotnet\sdk\5.0.100-preview.8.20417.9\NuGet.targets(128,5): error : Unable to load the service index for source https://api.nuget.org/v3/index.json. [C:\projects\sentry-dotnet\Sentry.sln]
C:\projects\sentry-dotnet\.tmp\dotnet\sdk\5.0.100-preview.8.20417.9\NuGet.targets(128,5): error :   No connection could be made because the target machine actively refused it. (api.nuget.org:443) [C:\projects\sentry-dotnet\Sentry.sln]
C:\projects\sentry-dotnet\.tmp\dotnet\sdk\5.0.100-preview.8.20417.9\NuGet.targets(128,5): error :   No connection could be made because the target machine actively refused it. [C:\projects\sentry-dotnet\Sentry.sln]
Command exited with code 1
bash scripts\zeus.sh report_failed

bruno-garcia · 2020-10-20T01:42:36Z

rerunning the build.
We'll hopefully soon move over to GHA completely

bruno-garcia

Good stuff!

bruno-garcia · 2020-10-20T18:11:31Z

Thanks again @PureKrome !

PureKrome · 2020-10-20T21:36:49Z

My pleasure 👍🏻

ASPNET Core default Environment settings are cleaned, if used.

86c403e

PureKrome requested a review from bruno-garcia as a code owner October 18, 2020 13:49

PureKrome changed the title ~~ASPNET Core default Environment settings are cleaned, if used.~~ ASPNET.Core default Environment settings are cleaned, if used. Oct 18, 2020

🖊️

db75b2e

xt0rted reviewed Oct 18, 2020

View reviewed changes

src/Sentry.AspNetCore/SentryAspNetCoreOptionsSetup.cs Outdated Show resolved Hide resolved

Update src/Sentry.AspNetCore/SentryAspNetCoreOptionsSetup.cs

3920a64

Co-authored-by: Brian Surowiec <[email protected]>

Another test to satisfy CodeCoverage.

91827ed

xt0rted reviewed Oct 19, 2020

View reviewed changes

test/Sentry.AspNetCore.Tests/SentryAspNetCoreOptionsSetupTests.cs Outdated Show resolved Hide resolved

Update test/Sentry.AspNetCore.Tests/SentryAspNetCoreOptionsSetupTests.cs

baba056

Co-authored-by: Brian Surowiec <[email protected]>

xt0rted reviewed Oct 19, 2020

View reviewed changes

test/Sentry.AspNetCore.Tests/SentryAspNetCoreOptionsSetupTests.cs Show resolved Hide resolved

💄

ff04d1a

Tyrrrz reviewed Oct 19, 2020

View reviewed changes

bruno-garcia reviewed Oct 19, 2020

View reviewed changes

src/Sentry.AspNetCore/SentryAspNetCoreOptionsSetup.cs Outdated Show resolved Hide resolved

src/Sentry.AspNetCore/SentryAspNetCoreOptionsSetup.cs Outdated Show resolved Hide resolved

PureKrome and others added 2 commits October 20, 2020 08:33

Update src/Sentry.AspNetCore/SentryAspNetCoreOptionsSetup.cs

18e825a

Co-authored-by: Bruno Garcia <[email protected]>

PR feedback.

f426efd

PureKrome force-pushed the 551/aspnet-core-fix-environment-settings branch from 40ad2f5 to f426efd Compare October 19, 2020 22:04

Tyrrrz approved these changes Oct 20, 2020

View reviewed changes

bruno-garcia approved these changes Oct 20, 2020

View reviewed changes

Merge branch 'main' into 551/aspnet-core-fix-environment-settings

890c095

bruno-garcia added the hacktoberfest-accepted label Oct 20, 2020

bruno-garcia merged commit 5d76498 into getsentry:main Oct 20, 2020

Turnerj mentioned this pull request Jun 6, 2021

Include staging in the environment naming rules from HostEnvironment #1046

Merged

PureKrome deleted the 551/aspnet-core-fix-environment-settings branch September 2, 2022 04:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ASPNET.Core default Environment settings are cleaned, if used. #554

ASPNET.Core default Environment settings are cleaned, if used. #554

PureKrome commented Oct 18, 2020 •

edited

Loading

PureKrome commented Oct 18, 2020

codecov-io commented Oct 18, 2020 •

edited

Loading

PureKrome commented Oct 19, 2020 •

edited

Loading

Tyrrrz commented Oct 19, 2020 •

edited

Loading

Tyrrrz Oct 19, 2020

PureKrome Oct 19, 2020 •

edited

Loading

bruno-garcia left a comment

PureKrome commented Oct 20, 2020

bruno-garcia commented Oct 20, 2020

bruno-garcia left a comment

bruno-garcia commented Oct 20, 2020

PureKrome commented Oct 20, 2020

ASPNET.Core default Environment settings are cleaned, if used. #554

ASPNET.Core default Environment settings are cleaned, if used. #554

Conversation

PureKrome commented Oct 18, 2020 • edited Loading

PureKrome commented Oct 18, 2020

codecov-io commented Oct 18, 2020 • edited Loading

Codecov Report

PureKrome commented Oct 19, 2020 • edited Loading

Tyrrrz commented Oct 19, 2020 • edited Loading

Tyrrrz Oct 19, 2020

Choose a reason for hiding this comment

PureKrome Oct 19, 2020 • edited Loading

Choose a reason for hiding this comment

bruno-garcia left a comment

Choose a reason for hiding this comment

PureKrome commented Oct 20, 2020

bruno-garcia commented Oct 20, 2020

bruno-garcia left a comment

Choose a reason for hiding this comment

bruno-garcia commented Oct 20, 2020

PureKrome commented Oct 20, 2020

PureKrome commented Oct 18, 2020 •

edited

Loading

codecov-io commented Oct 18, 2020 •

edited

Loading

PureKrome commented Oct 19, 2020 •

edited

Loading

Tyrrrz commented Oct 19, 2020 •

edited

Loading

PureKrome Oct 19, 2020 •

edited

Loading