#28390 API design (schedule script)#30443
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| "team_id": 123, | ||
| "starts_at": "2025-07-01T15:00:00Z", | ||
| "completed_at": "2025-07-06T15:00:00Z", | ||
| "status": "completed", |
There was a problem hiding this comment.
Dev note
Status can be completed, started, or scheduled.
There was a problem hiding this comment.
Update 8/7 - completed -> finished
| "team_id": 123, | ||
| "not_before": "2025-07-01T15:00:00Z", | ||
| "completed_at": "2025-07-06T15:00:00Z", | ||
| "status": "completed", |
There was a problem hiding this comment.
If a batch has "status": "started", can "not_before" be null in the case that the batch was run immediately? Seems like this should be the case, so seems like we also need a "started_at" field to be able to say when a non-scheduled batch run began, right?
There was a problem hiding this comment.
In fact, the status field is maybe redundant - that data can be derived from not_before, started_at, and completed_at:
!!completed_at -> completed, else !!started_at -> started, else !!not_before -> scheduled, else this batch summary shouldn't exist
There was a problem hiding this comment.
If we do keep status, seems canceled should be an option and not its own field, though better to remove status and update to canceled_at.
There was a problem hiding this comment.
Summary of all of these comments - proposal:
- Remove
"status"field - Update
"canceled"boolean field to"canceled_at"datetime string field - Add
"started_at"datetime string field
There was a problem hiding this comment.
Re: status you're not wrong that it can be derived, but I'd prefer to keep status as a property rather than have the front-end contain business logic to determine it. Kinda like how we have has_previous_results and has_next_results for pagination even though it can technically be figured out with a combination of page, page size and total count. +1 on adding started_at and canceled -> canceled_at.
There was a problem hiding this comment.
The reason I like canceled_at is so that completed_at isn't ambiguous. Another possibly better option is:
- rename
completed_attofinished_atand set it when a batch completes or is canceled - add "canceled" as a valid value for
status - remove the
canceledproperty (and don't addcanceled_at).
This way we have one field (finished_at) indicating when the job stopped running, and one field (state) indicating why.
There was a problem hiding this comment.
The canceled_at / completed_at approach dovetails with the "status" of a summary being completely defined by (not_before, started_at, completed_at, canceled_at) and not having a "status" field.
This suggestion to change to finished_at + add a "canceled" status dovetails with the current, "status"-field based approach. Both make sense to me, so given we want to keep status this proposal makes a lot of sense
There was a problem hiding this comment.
decision:
completed_at –> finished_at
status.completed -> status.finished
canceled flag remains
| "script_executed_at": "2024-09-11T20:30:24Z", | ||
| "script_output_preview": "hello world" |
There was a problem hiding this comment.
assuming these 2 fields null for pending/cancelled/incompatible ?
rachaelshaw
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
4 participants
For #28390