Update GetNextFinishedBuild endpoint to query based on build id#952
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #952 +/- ##
==========================================
+ Coverage 65.61% 65.65% +0.04%
==========================================
Files 381 381
Lines 21703 21758 +55
Branches 2770 2779 +9
==========================================
+ Hits 14241 14286 +45
- Misses 6425 6427 +2
- Partials 1037 1045 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Enkidu93
left a comment
There was a problem hiding this comment.
@Enkidu93 reviewed 4 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on ddaspit).
ddaspit
left a comment
There was a problem hiding this comment.
What happens if two builds are created at the same time down to the millisecond?
@ddaspit reviewed 4 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on pmachapman).
pmachapman
left a comment
There was a problem hiding this comment.
What happens if two builds are created at the same time down to the millisecond?
@ddaspit Only one would return. As it is the completion timestamp we look at, this should not occur due to the post processing job not running in parallel.
If you think this is an issue, probably the best way would be to supply an id of the last build the client received, and use >= instead. I can do this if you like?
@pmachapman made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on pmachapman).
33ab8cf to
01ba6c8
Compare
pmachapman
left a comment
There was a problem hiding this comment.
What happens if two builds are created at the same time down to the millisecond?
Based on our discussions this morning, I have added support for specifying the ID of the last completed build. I think the original PR change is still necessary for the other endpoint that uses DateTime.
To implement this change, I needed to add sorting support to the subscriptions. I thought I would make it a little bit more flexible by adding Ascending/Descending support (even though I only need ascending for this PR).
@pmachapman made 1 comment.
Reviewable status: 2 of 15 files reviewed, all discussions resolved (waiting on ddaspit and Enkidu93).
Enkidu93
left a comment
There was a problem hiding this comment.
@Enkidu93 reviewed 13 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on ddaspit and pmachapman).
src/Serval/src/Serval.Translation/Features/Builds/GetNextFinishedBuild.cs line 26 at r2 (raw file):
{ Build? build = await builds.GetAsync(id, cancellationToken); if (build is not null)
Just a little thing: This confused me for a moment since you end up setting it to DateTime.UtcNow again if the date is null. I wonder if
if (build is not null && build.DateFinished is not null)
dateFinished = build.DateFinished;
would be a little clearer?
ddaspit
left a comment
There was a problem hiding this comment.
@ddaspit reviewed 13 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on pmachapman).
- Add sort support to subscriptions - Add index to date finished field - Improve build id declarations in unit tests
01ba6c8 to
24f89e3
Compare
This PR fixes a bug I noticed where the date time values passed to the
GetAllBuildsCreatedAfterAsyncendpoint were missing millisecond values, and so the same build was being returned each time.I also updated the method documentation to remove the outdated only in UTC requirement.
This change is