Refs #10755 - Recurring actions, job invocation task groups #44

adamruzicka · 2015-09-24T15:20:31Z

theforeman-bot · 2015-10-05T13:05:22Z

@adamruzicka, this pull request is currently not mergeable. Please rebase against the master branch and push again.

If you have a remote called 'upstream' that points to this repository, you can do this by running:

    $ git pull --rebase upstream master

This message was auto-generated by Foreman's prprocessor

ares · 2015-10-05T13:06:28Z

request for rebase :-)

iNecas · 2015-10-05T18:46:32Z

app/controllers/job_invocations_controller.rb

@@ -31,6 +31,11 @@ def create
        ForemanTasks.delay action,
                           job_invocation.delay_options,
                           job_invocation
+      elsif job_invocation.trigger_mode == :recurring


case job_invocation.trigger_mode might work better than if/elsif here

theforeman-bot · 2015-10-06T09:50:21Z

@adamruzicka, this pull request is currently not mergeable. Please rebase against the master branch and push again.

If you have a remote called 'upstream' that points to this repository, you can do this by running:

    $ git pull --rebase upstream master

This message was auto-generated by Foreman's prprocessor

iNecas · 2015-10-21T19:07:58Z

Did another set of testing, the new version, with middleware works much intuitively.

iNecas · 2015-10-21T19:13:09Z

app/lib/actions/remote_execution/run_hosts_job.rb


-      def delay(delay_options, job_invocation)
+      def delay(delay_options, job_invocation, locked = false)
+        task.add_missing_task_groups(job_invocation.task_group)


Currently, the iterations after the first one don't have their own job invocation instance, so that the events are not shown in job invocations index, so we then can't track status from the job perspective then. We should check here, and in case the job has already a task, we should clone the invocation, rather than use the current one.

Also, since we're mapping the job invocation to 0-1 tasks, we should rename 'last_task_id', to just 'task_id'.

iNecas · 2015-10-21T19:17:17Z

It should be nice, to be able to get to the recurring logic details, where one would be able to get to the next jobs of belonging the the recurring logic group etc.

ares · 2015-10-27T15:46:09Z

This needs rebase :-(

adamruzicka · 2015-11-16T12:12:16Z

@iNecas all the comments should be addressed

iNecas · 2015-11-20T13:21:35Z

When using hourly recurring option, I'm getting:

invalid strptime format - `%Y-%m-%d %H:%M'
Rails.root: /home/inecas/Projects/ws/foreman-rex/foreman

/home/inecas/Projects/ws/foreman-rex/foreman-tasks/app/models/foreman_tasks/triggering.rb:78:in `parse_start_at!'
/home/inecas/Projects/ws/foreman-rex/foreman-tasks/app/models/foreman_tasks/triggering.rb:9:in `block in <class:Triggering>'
/home/inecas/Projects/ws/foreman-rex/foreman_remote_execution/app/models/job_invocation_composer.rb:45:in `save'
/home/inecas/Projects/ws/foreman-rex/foreman_remote_execution/app/controllers/job_invocations_controller.rb:31:in `create'
app/controllers/concerns/application_shared.rb:13:in `set_timezone'
app/models/concerns/foreman/thread_session.rb:32:in `clear_thread'
lib/middleware/catch_json_parse_errors.rb:9:in `call'

Debugging showed that the start_at_raw was empty.

iNecas · 2015-11-20T13:29:44Z

db/migrate/20150923125825_add_job_invocation_task_group.rb

@@ -0,0 +1,9 @@
+class AddJobInvocationTaskGroup < ActiveRecord::Migration
+  def up
+    add_column :job_invocations, :task_group_id, :integer, :index => true


Foreign key?

Yes. Should I add the database constraint or can we allow a job invocation to have no task group?

AFAIK you can have foreign key on nullable fields - it should check the contraint just on the case, when the field is entered.

You're right, I'll add the constraints

adamruzicka · 2015-11-20T13:30:00Z

Should be fixed in adamruzicka/foreman-tasks@20cf8ea

iNecas · 2015-11-20T13:30:02Z

db/migrate/20151116105412_add_triggering_to_job_invocation.rb

@@ -0,0 +1,9 @@
+class AddTriggeringToJobInvocation < ActiveRecord::Migration
+  def up
+    add_column :job_invocations, :triggering_id, :integer, :index => true


Foreign key again?

Again, yes. A job invocation could be created without a triggering so it made sense to me not to enforce the association

iNecas · 2015-11-23T19:35:21Z

Looks better every iteration: from the last testing (some comments might apply to the foreman-tasks PR):

future execution doesn't have a default values set - this was quite useful to have set to current time, instead of having to write the time on my own, I would welcome having that back
related resources in the job invocation seems strange (in the recurring logics it might have some sense, although even there, it would be nice just to be able to link to the job invocation detail), having 'Recurring logic' tab there would be better IMO
canceling the recurring logic leads to 'Template missing' error message
Finished: running seems strange, status might be better, also, I would perhaps choose Active over Running, as the Running might feel the task actually running at the moment and is a bit misleading

adamruzicka · 2015-11-26T10:59:20Z

future execution doesn't have a default values set - this was quite useful to have set to current time, instead of having to write the time on my own, I would welcome having that back

canceling the recurring logic leads to 'Template missing' error message

Finished: running seems strange, status might be better, also, I would perhaps choose Active over Running, as the Running might feel the task actually running at the moment and is a bit misleading

addressed in adamruzicka/foreman-tasks@8ee423e

related resources in the job invocation seems strange (in the recurring logics it might have some sense, although even there, it would be nice just to be able to link to the job invocation detail), having 'Recurring logic' tab there would be better IMO

In recurring logic details

Shows type of the resource and count per type, links to job invocations index filtered by the recurring logic id

In job invocation details

Should this show the resources related to the recurring logic too?

iNecas · 2015-11-26T13:38:34Z

The default time for future execution works for new invocations, but still missing when using the re-run. Minor, but still nice to have enhancement.

iNecas · 2015-11-27T13:14:22Z

Waiting for theforeman/foreman-tasks#141 to get released

iNecas · 2015-12-01T14:48:58Z

Waiting for rubocop and squash

iNecas · 2015-12-02T11:31:41Z

No other reasons for not getting this into master. Merging now. Thanks @adamruzicka

Refs #10755 - Recurring actions, job invocation task groups

theforeman-bot added Needs testing Not yet reviewed labels Sep 24, 2015

theforeman-bot added Waiting on contributor and removed Needs testing Needs re-review Not yet reviewed labels Oct 5, 2015

iNecas reviewed Oct 5, 2015
View reviewed changes

theforeman-bot added Needs testing Needs re-review Waiting on contributor and removed Waiting on contributor Needs testing Needs re-review Not yet reviewed labels Oct 6, 2015

adamruzicka force-pushed the recurring branch from 8ada2dc to 9d43041 Compare October 6, 2015 10:36

theforeman-bot added Needs testing Needs re-review and removed Waiting on contributor labels Oct 6, 2015

adamruzicka force-pushed the recurring branch from 2ec2fb6 to 7496e12 Compare October 19, 2015 10:23

iNecas reviewed Oct 21, 2015
View reviewed changes

ares added Waiting on contributor and removed Needs re-review labels Oct 27, 2015

theforeman-bot added Needs testing Needs re-review Waiting on contributor and removed Waiting on contributor Needs testing Needs re-review Not yet reviewed labels Nov 16, 2015

adamruzicka force-pushed the recurring branch from 629a48e to 68ef136 Compare November 16, 2015 12:11

theforeman-bot added Needs testing Needs re-review and removed Waiting on contributor labels Nov 16, 2015

iNecas reviewed Nov 20, 2015
View reviewed changes

adamruzicka force-pushed the recurring branch 2 times, most recently from 1bf6411 to 1e56517 Compare December 1, 2015 12:09

Refs #10755 - Task groups and recurring logic

2fa2ed3

adamruzicka force-pushed the recurring branch from 416a537 to 2fa2ed3 Compare December 1, 2015 16:43

iNecas added a commit that referenced this pull request Dec 2, 2015

Merge pull request #44 from adamruzicka/recurring

03807a0

Refs #10755 - Recurring actions, job invocation task groups

iNecas merged commit 03807a0 into theforeman:master Dec 2, 2015

adamruzicka deleted the recurring branch December 2, 2015 16:57

adamruzicka mentioned this pull request Dec 3, 2015

Fixes unknown column errors on last_task #79

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refs #10755 - Recurring actions, job invocation task groups #44

Refs #10755 - Recurring actions, job invocation task groups #44

adamruzicka commented Sep 24, 2015

theforeman-bot commented Oct 5, 2015

ares commented Oct 5, 2015

iNecas Oct 5, 2015

theforeman-bot commented Oct 6, 2015

iNecas commented Oct 21, 2015

iNecas Oct 21, 2015

iNecas commented Oct 21, 2015

ares commented Oct 27, 2015

adamruzicka commented Nov 16, 2015

iNecas commented Nov 20, 2015

iNecas Nov 20, 2015

adamruzicka Nov 20, 2015

iNecas Nov 20, 2015

adamruzicka Nov 20, 2015

adamruzicka commented Nov 20, 2015

iNecas Nov 20, 2015

adamruzicka Nov 20, 2015

iNecas commented Nov 23, 2015

adamruzicka commented Nov 26, 2015

iNecas commented Nov 26, 2015

iNecas commented Nov 27, 2015

iNecas commented Dec 1, 2015

iNecas commented Dec 2, 2015

Refs #10755 - Recurring actions, job invocation task groups #44

Refs #10755 - Recurring actions, job invocation task groups #44

Conversation

adamruzicka commented Sep 24, 2015

theforeman-bot commented Oct 5, 2015

ares commented Oct 5, 2015

Choose a reason for hiding this comment

theforeman-bot commented Oct 6, 2015

iNecas commented Oct 21, 2015

Choose a reason for hiding this comment

iNecas commented Oct 21, 2015

ares commented Oct 27, 2015

adamruzicka commented Nov 16, 2015

iNecas commented Nov 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamruzicka commented Nov 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iNecas commented Nov 23, 2015

adamruzicka commented Nov 26, 2015

In recurring logic details

In job invocation details

iNecas commented Nov 26, 2015

iNecas commented Nov 27, 2015

iNecas commented Dec 1, 2015

iNecas commented Dec 2, 2015