Repository navigation
Conversation
And bump Postgres version used to 18.
And because the output is now isolated, hijack the ContentRoot early enough to prevent the test host from complaining.
Summary
Skipped
🎉 No failed tests in this run. Github Test Reporter by CTRF 💚 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v11.0-preview #896 +/- ##
=================================================
- Coverage 85.41% 85.34% -0.07%
=================================================
Files 246 246
Lines 5210 5227 +17
Branches 385 385
=================================================
+ Hits 4450 4461 +11
- Misses 676 681 +5
- Partials 84 85 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
tomaszlaskowskiLC
left a comment
tomaszlaskowskiLC
left a comment
There was a problem hiding this comment.
@Saancreed I agree on waiting until the package issue is resolved. Is there expected timeline for that?
| ) | ||
| { | ||
| message.Tokens = await tokenStore.GetTokensAsync(userId, cancellationToken); | ||
| message.Fids = await tokenStore.GetTokensAsync(userId, cancellationToken); |
There was a problem hiding this comment.
TIL, that Token is getting obsolete. 😛
Anyway, it's not obvious that you can pass Tokens as Fids.
If we would like to keep "Tokens" everywhere else and use Fids property in this client, we should add comment with an explanation here and mention it in the docs.
Also, let's ensure there is no changes required on the client side (I mean mobile apps 😛).
There was a problem hiding this comment.
Ah, good point. I initially found https://firebase.google.com/docs/projects/manage-installations#retrieving-an-fcm-registration-token which made me think "ok, so it's fine to keep using FirebaseMessaging.getInstance().token", but it appears that it's kinda misleading and this isn't actually related to migration to FIDs.
We may want to keep using Tokens for some time, as it's documented to also accept FIDs during the migration period… whatever that is.
| new AppOptions | ||
| { | ||
| Credential = GoogleCredential.FromJson(cfg), | ||
| Credential = CredentialFactory.FromJson<ServiceAccountCredential>(cfg).ToGoogleCredential(), |
There was a problem hiding this comment.
NIT: technically this narrows accepted credentials to service accounts (although it should be the way-to-go here).
Might be worth noting in the docs.
There was a problem hiding this comment.
Noting, yes. Notice how the changelog entries for v11 are still missing, it will be a good place to put it there once it actually materializes.
| @@ -97,36 +103,36 @@ | |||
| <PackageVersion Include="Microsoft.Extensions.Options.ConfigurationExtensions" Version="$(ExtensionsVersion)" /> | |||
|
|
|||
| <PackageVersion Include="Microsoft.Extensions.Telemetry.Testing" Version="8.0.0-rc.2.23510.2" /> | |||
There was a problem hiding this comment.
No bump for this one?
There was a problem hiding this comment.
This is already the newest version: https://www.nuget.org/packages/Microsoft.Extensions.Telemetry.Testing
I'm not sure what we need this for, but the package appears to be kind of abandoned. Didn't want to touch this during the bump though.
| ) : IntegrationFactAttribute(sourceFilePath, sourceLineNumber) | ||
| public sealed class PostgresOnlyFactAttribute : IntegrationFactAttribute | ||
| { | ||
| public static bool IsPostgres => Environment.GetEnvironmentVariable(TestDatabaseConfig.ConfigEnvName) == "postgres"; |
There was a problem hiding this comment.
NIT: I guess IsPostgres belongs more to TestDatabaseConfig.
There was a problem hiding this comment.
Sure, can move it there.
| ConvertDateTimeBySystemTimeZoneIdMethod, | ||
| Expression.Property(Expression.Property(node.Expression, UtcTimestampProperty), UtcDateTimeProperty), | ||
| // Npgsql 11 translates UtcDateTime as a no-op which retains DateTimeOffset as the SQL | ||
| // expression's CLR type. ToUniversalTime retags it as DateTime, which the outer translator requires. |
There was a problem hiding this comment.
You can also link your issue here (shows blocker explicitly): npgsql/efcore.pg#3912. 😄
There was a problem hiding this comment.
IIRC I committed that before I made this issue 🙃
Nothing confirmed yet, but I'd expect rc.2 to address that. If not, we'll have to wait for 11.0.0 to be released. |
Unfortunately, the Npgsql shenanigans which require us to pull packages from daily .NET package feed make me very reluctant to merge this before upstream resolves this in a reasonable manner. See the "Update integration tests to .NET 11 and Npgsql.EFCore to rc.1" and "Update Extensions version to satisfy EFCore requirements" for details.