Draft: Introduce new initial job state "new" #6157
Conversation
This draft shows what places one needed to adjust just for the sake of having a new state at all. What is still missing: * Several tests need to be adjusted. * The actual database migration needs to be provided. * The transition from "new" to "scheduled" when jobs are created or when related Minion jobs have finished. Related ticket: https://progress.opensuse.org/issues/176067
|
Great PR! Please pay attention to the following items before merging: Files matching
Files matching
This is an automatically generated QA checklist based on modified files. |
| @@ -75,7 +75,7 @@ function renderJobResults(data, type, row) { | |||
| // job status | |||
| html += '<span id="res-' + row.id + '">'; | |||
| html += '<a href="' + urlWithBase('/tests/' + row.id) + '">'; | |||
| if (row.result == 'none' && (row.state == 'running' || row.state == 'scheduled')) { | |||
| if (row.result == 'none' && (row.state === 'running' || row.state === 'scheduled' || row.state === 'new')) { | |||
There was a problem hiding this comment.
can we keep the state "new" as a substate internally but treat it the same as scheduled for the whole UI layer including here?
There was a problem hiding this comment.
We could add yet another column to the jobs table (besides the relation for referencing Gru tasks) that allows us to flag jobs as not ready to be scheduled (without already having to assign a gru job ID).
Maybe we could also create a dummy Gru task without Minion job and assign that initially. This approach would avoid the extra column but also be a bit hacky.
There was a problem hiding this comment.
I was thinking about forward compatibility and options to extend for potential future use cases. job->state is already a flexible "varchar" in the schema so we can also reflect "new" there but here and in other cases we are in the UI layer so I thought that maybe we can translate from internal states to external states. We already have layed the groundwork with lib/OpenQA/Jobs/Constants.pm the past years where we have meta results and meta results. How about keeping "state" strictly internal – or maybe only reflect over the API for expert usage – and keep the UI layer strictly bound to the meta states?
| @@ -136,7 +136,7 @@ sub _incomplete_previous_job { | |||
| my $job_id = $job->id; | |||
| return 0 if $jobs_worker_says_it_works_on->{$job_id}; | |||
| my $job_state = $job->state; | |||
| return 1 if $job_state eq OpenQA::Jobs::Constants::SCHEDULED; | |||
| return 1 if $job_state eq NEW || $job_state eq SCHEDULED; | |||
There was a problem hiding this comment.
I think in all logic we should avoid "this state || that state" and instead use aggregated states where we would need to check for more than one state. Same as you did in https://github.com/os-autoinst/openQA/pull/6157/files#diff-1c4e8d9bc0096026bc67314675374924c7a5da02359b426a1c6401eac9e90abeR1814
| @@ -29,7 +29,7 @@ | |||
| </div> | |||
|
|
|||
| <div> | |||
| <h2 id="scheduled_jobs_heading">Scheduled jobs</h2> | |||
| <h2 id="scheduled_jobs_heading">New and Scheduled jobs</h2> | |||
There was a problem hiding this comment.
Same as my comment in https://github.com/os-autoinst/openQA/pull/6157/files#diff-6937e1376520bc4c4992d9cf39d5a3522676f0ae07e4be058fe88407e717f266R78 I think it would be good to distinguish between UI facing states and sub-states in the implementation unless there is a difference for the user. So far I don't see that difference for the user.
| % elsif ($state eq SCHEDULED) { | ||
| % my $substate = $res->{blocked} ? 'blocked' : 'scheduled'; | ||
| % elsif ($state eq NEW || $state eq SCHEDULED) { | ||
| % my $substate = $res->{blocked} ? 'blocked' : $state; |
There was a problem hiding this comment.
IMHO this "substate" is already a better approach for the UI level
This draft shows what places one needed to adjust just for the sake of having a new state at all.
What is still missing:
Related ticket: https://progress.opensuse.org/issues/176067