studio/flamenco
studio/flamenco
12
30
Fork 38
Code Issues 63 Pull Requests 5 Releases Wiki Activity

Allow jobs to be submitted in paused status #104322

Merged
David Zhang merged 3 commits from David-Zhang-10/flamenco:submit-as-paused into main 2024-07-11 17:08:27 +02:00
Conversation 1 Commits 3 Files Changed 11 +6 -2
1 changed files with 6 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 00848a3755 - Show all commits

8
internal/manager/api_impl/jobs.go
View File

@ -91,8 +91,12 @@ func (f *Flamenco) SubmitJob(e echo.Context) error {
logger = logger.With().Str("job_id", authoredJob.JobID).Logger()
// TODO: check whether this job should be queued immediately or start paused.
authoredJob.Status = api.JobStatusQueued
submittedJob := api.SubmittedJob(job)
initialStatus := api.JobStatusQueued
David-Zhang-10 marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2024-07-08 15:20:06 +02:00
Outdated
Review
Copy Link

If submittedJob.InitialStatus is nil, dereferencing the pointer will cause a panic.

If `submittedJob.InitialStatus` is `nil`, dereferencing the pointer will cause a panic.
if submittedJob.InitialStatus != nil {
initialStatus = *submittedJob.InitialStatus
}
David-Zhang-10 marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2024-07-08 15:26:48 +02:00
Outdated
Review
Copy Link

This will cause a job with initialStatus: "failed" to be accepted, and silently its initial status will be overwritten. I don't this is a good approach. I think this is sufficient:

initialStatus := api.JobStatusQueued
if (submittedJob.InitialStatus != nil) {
    initialStatus = *submittedJob.InitialStatus
}
authoredJob.Status = initialStatus

If you want to do more validation you could, but I don't think it's necessary. In that case, a global variable declared at the top could help:

var validInitialJobStatuses = map[api.JobStatus]bool{
	api.JobStatusQueued: true,
	api.JobStatusPaused: true,
}

// ...

if !validInitialJobStatuses[initialStatus] {
  // .. reject submitted job with appropriate HTTP error status code
}
This will cause a job with `initialStatus: "failed"` to be accepted, and silently its initial status will be overwritten. I don't this is a good approach. I think this is sufficient: ```go initialStatus := api.JobStatusQueued if (submittedJob.InitialStatus != nil) { initialStatus = *submittedJob.InitialStatus } authoredJob.Status = initialStatus ``` If you want to do more validation you could, but I don't think it's necessary. In that case, a global variable declared at the top could help: ```go var validInitialJobStatuses = map[api.JobStatus]bool{ api.JobStatusQueued: true, api.JobStatusPaused: true, } // ... if !validInitialJobStatuses[initialStatus] { // .. reject submitted job with appropriate HTTP error status code } ```
authoredJob.Status = initialStatus
if err := f.persist.StoreAuthoredJob(ctx, *authoredJob); err != nil {
logger.Error().Err(err).Msg("error persisting job in database")