SF-3808 Stop using the webhook#3920
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3920 +/- ##
==========================================
+ Coverage 80.88% 80.90% +0.01%
==========================================
Files 634 636 +2
Lines 41017 41065 +48
Branches 6654 6672 +18
==========================================
+ Hits 33177 33222 +45
+ Misses 6802 6792 -10
- Partials 1038 1051 +13 ☔ View full report in Codecov by Harness. |
Nateowami
left a comment
There was a problem hiding this comment.
@Nateowami made 1 comment.
Reviewable status: 0 of 46 files reviewed, 1 unresolved discussion (waiting on pmachapman).
.github/workflows/release.yml line 180 at r2 (raw file):
PARATEXT_RESOURCE_PASSWORD_BASE64: ${{secrets.paratext_resource_password_base64}} PARATEXT_RESOURCE_PASSWORD_HASH: ${{secrets.paratext_resource_password_hash}} SERVAL_CLIENT_SECRET: ${{secrets.serval_client_secret
Missing }} at end of line
Nateowami
left a comment
There was a problem hiding this comment.
@Nateowami reviewed 1 file and made 1 comment.
Reviewable status: 1 of 46 files reviewed, 2 unresolved discussions (waiting on pmachapman).
src/SIL.XForge/DataAccess/MemoryRepository.cs line 178 at r2 (raw file):
public Task<T> DeleteAsync(Expression<Func<T, bool>> filter, CancellationToken _ = default) { T entity = Query().First(filter);
What's the purpose of this change?
MongoRepository calls collection.FindOneAndDeleteAsync, which says it returns The deleted document if one was deleted., implying that it doesn't throw if the document isn't matched, whereas .First(filter) is going to throw an exception. Seems like our tests will meaningfully diverge.
pmachapman
left a comment
There was a problem hiding this comment.
@pmachapman made 2 comments and resolved 1 discussion.
Reviewable status: 0 of 48 files reviewed, 1 unresolved discussion (waiting on Nateowami).
.github/workflows/release.yml line 180 at r2 (raw file):
Previously, Nateowami wrote…
Missing
}}at end of line
Done. Thank you!
src/SIL.XForge/DataAccess/MemoryRepository.cs line 178 at r2 (raw file):
Previously, Nateowami wrote…
What's the purpose of this change?
MongoRepositorycallscollection.FindOneAndDeleteAsync, which says it returnsThe deleted document if one was deleted., implying that it doesn't throw if the document isn't matched, whereas.First(filter)is going to throw an exception. Seems like our tests will meaningfully diverge.
Done. Yes I think so to - I have reverted this. I wanted it to throw if a doc was not found (i.e. to show if a test was not deleting the expected document), but I agree that the diverging from Mongo is a bridge too far. I've updated the interface and Mongo implementation to match the nullability requirements.
RaymondLuong3
left a comment
There was a problem hiding this comment.
@RaymondLuong3 reviewed 32 files and all commit messages.
Reviewable status: 32 of 48 files reviewed, 1 unresolved discussion (waiting on Nateowami).
Nateowami
left a comment
There was a problem hiding this comment.
@Nateowami resolved 1 discussion.
Reviewable status: 32 of 48 files reviewed, all discussions resolved (waiting on RaymondLuong3).
4510159 to
a3a4f39
Compare
RaymondLuong3
left a comment
There was a problem hiding this comment.
@RaymondLuong3 reviewed 18 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on pmachapman).
test/SIL.XForge.Scripture.Tests/Services/MachineApiServiceTests.cs line 3904 at r4 (raw file):
JobState.Completed, CancellationToken.None );
Does this test need to say 'ProcessBuildAsync_InvalidTranslationEngineId'? It looks like this is really testing if the translation engine ID is invalid.
Code quote:
await env.Service.ProcessBuildAsync(
"invalid_translation_id",
ServalBuildId01,
JobState.Completed,
CancellationToken.None
);test/SIL.XForge.Scripture.Tests/Services/MachineBackgroundServiceTests.cs line 77 at r4 (raw file):
// Confirm initial environment long siteConfigCount = await env.SiteConfigs.CountDocumentsAsync(_ => true, CancellationToken.None);
Nit: did you mean for this type to be int?
Code quote:
long siteConfigCount = await env.SiteConfigs.CountDocumentsAsync(_ => true, CancellationToken.None);
Nateowami
left a comment
There was a problem hiding this comment.
@Nateowami made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on pmachapman and RaymondLuong3).
test/SIL.XForge.Scripture.Tests/Services/MachineBackgroundServiceTests.cs line 77 at r4 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
Nit: did you mean for this type to be
int?
@RaymondLuong3 I think it's because CountDocumentsAsync returns long, and it's simpler to just stick with long than to try to cast it to int.
pmachapman
left a comment
There was a problem hiding this comment.
@pmachapman made 1 comment and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on pmachapman).
test/SIL.XForge.Scripture.Tests/Services/MachineApiServiceTests.cs line 3904 at r4 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
Does this test need to say 'ProcessBuildAsync_InvalidTranslationEngineId'? It looks like this is really testing if the translation engine ID is invalid.
Done. Thank you!
RaymondLuong3
left a comment
There was a problem hiding this comment.
@RaymondLuong3 reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on pmachapman).
test/SIL.XForge.Scripture.Tests/Services/MachineBackgroundServiceTests.cs line 77 at r4 (raw file):
Previously, Nateowami wrote…
@RaymondLuong3 I think it's because CountDocumentsAsync returns long, and it's simpler to just stick with long than to try to cast it to int.
That makes sense. I was confused since it didn't seem like CountDocumentsAsync should return a decimal number, but makes sense this could be a big number.
This PR replaces the webhook with a background service that polls the new
GetNextFinishedBuildAsyncendpoint in Serval 1.18.0. I am marking this PR as do not merge, as this endpoint is currently only on Serval QA.This change is