-
Notifications
You must be signed in to change notification settings - Fork 210
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft: Introduce new initial job state "new" #6157
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
# set jobs which were only assigned anyways back to scheduled | ||
if ($job_state eq OpenQA::Jobs::Constants::ASSIGNED) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
<table id="scheduled" class="display table table-striped" style="width: 100%"> | ||
<thead> | ||
<tr> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,8 +21,8 @@ | |
<i class="status fa fa-circle result_<%= $res->{overall} %>" title="Done: <%= $res->{overall} %>"></i> | ||
</a> | ||
% } | ||
% 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMHO this "substate" is already a better approach for the UI level |
||
<a href="<%= url_for('test', 'testid' => $jobid) %>"> | ||
<i class="status state_<%= $substate %> fa fa-circle" title="<%= $substate %>@<%= $res->{priority} %>"></i> | ||
</a> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?