feat: Archive & Unarchive run #9143
Conversation
✅ Deploy Preview for determined-ui canceled.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9143 +/- ##
==========================================
+ Coverage 44.71% 44.74% +0.02%
==========================================
Files 1270 1270
Lines 155328 155486 +158
Branches 2436 2436
==========================================
+ Hits 69461 69565 +104
- Misses 85631 85685 +54
Partials 236 236
Flags with carried forward coverage won't be shown. Click here to find out more.
|
master/internal/api_runs.go
Outdated
| @@ -33,6 +34,7 @@ type archiveRunOKResult struct { | |||
| ID int32 | |||
| ExpID *int32 | |||
| IsMultitrial bool | |||
| State *bool | |||
There was a problem hiding this comment.
can you try this with sql.Null[bool] and see how it goes? this is for two reasons: we're focusing more on reliability and continue to see nil pointer exceptions a lot. and that is exactly what this is supposed to help, since sql.Null (or if we had it.. Option[T].. jealous of rust) is basically (better) compile time null safety.
There was a problem hiding this comment.
I think it may be better to have this be a bool. It still works in areas where the field is not being used.
| if filter == nil { | ||
| query = query.Where("r.id IN (?)", bun.In(runIDs)) | ||
| } else { | ||
| query, err = filterRunQuery(query, filter) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
nit, interface clarify. it's not clear to be from the signature that this is either<runIDs []int32, filter *string>. maybe add a func comment? once again, gah rust for having either<...>.
There was a problem hiding this comment.
or, as i'm reading this, since it has if filter == nil in a few places, maybe it just makes sense as two functions. and then you try to not repeat yourself some other way.
There was a problem hiding this comment.
Added error for cases when RunIDs and Filter are both filled. Will add this behaviour for other endpoints in their respective PRs
master/internal/api_runs.go
Outdated
| switch { | ||
| case check.Archived && archive: | ||
| results = append(results, &apiv1.RunActionResult{ | ||
| Error: "Run is already archived.", |
There was a problem hiding this comment.
why would this be an error? it seems like it would only cause confusion if it is surfaced, not clarity. and i think we should make all new api calls idempotent, it's a really useful property when building distributed systems. wdyt @ashtonG?
There was a problem hiding this comment.
The existing ArchiveExperiments & UnarchiveExperiments do this, so I just wanted to keep inline with the expected behaviour of an Archive action. On further investigation it seems as though ArchiveProject & ArchiveWorkspace return success if they are already archived. So we already have some inconsistencies. I agree that it should return a success
| return nil, err | ||
| } | ||
| for _, acceptID := range acceptedIDs { | ||
| results = append(results, &apiv1.RunActionResult{ |
There was a problem hiding this comment.
what about for stuff in valid IDs but not in accepted? is that technically possible if something is deleted during this flow.
an aside: I think we should be using db transactions for all these endpoints?
There was a problem hiding this comment.
second aside: make sure the frontend is ok if it doesn't end up getting a result for every row. or update your tests to check this specifically if they don't already?
There was a problem hiding this comment.
Don't believe it's an issue if a run is already deleted and we don't show the result in the response. In the case where a filter is used then it would actually be the preferred behaviour
There was a problem hiding this comment.
I don't understand the other cases then? Why have we been so particular about showing a response if it doesn't matter now?
Description
Adding ability to archive and unarchive runs
Test Plan
integration tests should pass
Checklist
docs/release-notes/.See Release Note for details.
Ticket
ET-68