-
-
Notifications
You must be signed in to change notification settings - Fork 226
Update all samples to use the latest TFMs #4782
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
base: main
Are you sure you want to change the base?
Conversation
Note that I can't test this (my Apple Developer account has expired)... hoping Stefan can help :-)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4782 +/- ##
==========================================
- Coverage 73.90% 73.88% -0.02%
==========================================
Files 485 485
Lines 17682 17682
Branches 3494 3494
==========================================
- Hits 13067 13064 -3
- Misses 3758 3763 +5
+ Partials 857 855 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
The error we're seeing now in CI is: This may be related to: @Flash0ver potentially we revert to net9 for the WASM sample until this is a bit more stable in .net 10? |
samples/Sentry.Samples.AspNetCore.Blazor.Wasm/Sentry.Samples.AspNetCore.Blazor.Wasm.csproj
Show resolved
Hide resolved
| #if SENTRY_DSN_DEFINED_IN_ENV | ||
| // A DSN is required. You can set here in code. Web browsers cannot read environment variables. | ||
| // See https://docs.sentry.io/product/sentry-basics/dsn-explainer/ | ||
| options.Dsn = SamplesShared.Dsn; | ||
| options.Dsn = EnvironmentVariables.Dsn; | ||
| #endif |
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.
note: Fixes WASM DSN issue
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.
@Flash0ver after reading your fix, I remembered why we have that EnvironmentVariables class, so I tweaked/updated to be consistent with how we've done this in the mobile samples.
sentry-dotnet/samples/Sentry.Samples.Maui/MauiProgram.cs
Lines 17 to 28 in 613a18d
| #if !SENTRY_DSN_DEFINED_IN_ENV | |
| // You must specify a DSN. On mobile platforms, this should be done in code here. | |
| // See https://docs.sentry.io/product/sentry-basics/dsn-explainer/ | |
| options.Dsn = SamplesShared.Dsn; | |
| #else | |
| // To make things easier for the SDK maintainers we have a custom build target that writes the | |
| // SENTRY_DSN environment variable into an EnvironmentVariables class that is available for mobile | |
| // targets. This allows us to share one DSN defined in the ENV across desktop and mobile samples. | |
| // Generally, you won't want to do this in your own mobile projects though - you should set the DSN | |
| // in code as above | |
| options.Dsn = EnvironmentVariables.Dsn; | |
| #endif |
|
Inexplicably, this commit seems to have broken the unit tests on Possibly the commit is entirely unrelated but, if so, I can't see any other differences between the environments between the successful and unsuccessful runs. Commit 6722643 confirms it's nothing to do with the change, but we do finally get some clues from the logs:
|
| // targets. This allows us to share one DSN defined in the ENV across desktop and mobile samples. | ||
| // Generally, you won't want to do this in your own WASM applications - you should set the DSN | ||
| // in code as above | ||
| options.Dsn = EnvironmentVariables.Dsn; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Resolves #4584
#skip-changelog