cookpad / kuroko2

Kuroko2 is a web-based job scheduler / workflow engine.
MIT License
313 stars 72 forks source link

Cannot create job definition #125

Open yohfee opened 5 years ago

yohfee commented 5 years ago

Ruby: 2.5.3 Rails: 5.1.6 Kuroko2: 0.5.0

Creating a new job definition responds "Revisions is invalid". It looks association between job_definition and revision is not set correctly according to simulating controller action with rails console. In spite of controllers and features specs success.

irb> d = Kuroko2::JobDefinition.new(name: 'Job', script: 'noop:', admins: [Kuroko2::User.first])
  Kuroko2::User Load (2.9ms)  SELECT  `kuroko2_users`.* FROM `kuroko2_users` ORDER BY `kuroko2_users`.`id` ASC LIMIT 1
=> #<Kuroko2::JobDefinition id: nil, version: 0, name: "Job", description: "An description of the job definition.\n\n## Failure ...", script: "noop:", suspended: false, prevent_multi: 1, notify_cancellation: true, hipchat_room: "", hipchat_notify_finished: true, hipchat_additional_text: nil, slack_channel: "", api_allowed: false, created_at: nil, updated_at: nil, webhook_url: nil>
irb> d.valid?
=> true
irb> d.save_and_record_revision
   (1.5ms)  BEGIN
   (0.9ms)  ROLLBACK
=> false
irb> d.revisions.size
=> 1
irb> d.revisions.first.errors.full_messages
=> ["Job definition must exist"]
irb> d.revisions.first.job_definition
=> nil

I think one of below ideas may help (sorry for not PR).

  1. Add inverse_of option to revisions association. https://github.com/cookpad/kuroko2/blob/master/app/models/kuroko2/job_definition.rb#L33

    has_many :revisions, ..., inverse_of: :job_definition
  2. Set association explicitly. https://github.com/cookpad/kuroko2/blob/master/app/models/kuroko2/job_definition.rb#L145

    revisions.new(job_definition: self, ...)

Latter one have less impact on app. How do you think?

eagletmt commented 5 years ago

Hmm, I couldn't reproduce your situation.

% bin/rails c --sandbox
Loading development environment in sandbox (Rails 5.1.6)
Any modifications you make will be rolled back on exit
[1] pry(main)> u = Kuroko2::User.active.first; nil
  Kuroko2::User Load (0.4ms)  SELECT  `users`.* FROM `users` WHERE `users`.`suspended_at` IS NULL ORDER BY `users`.`id` ASC LIMIT 1
=> nil
[2] pry(main)> d = Kuroko2::JobDefinition.new(name: 'job', script: 'noop:'); nil
=> nil
[3] pry(main)> d.admins = [u]; nil
=> nil
[4] pry(main)> d.valid?
=> true
[5] pry(main)> d.save_and_record_revision
   (0.3ms)  SAVEPOINT active_record_1
  SQL (0.4ms)  INSERT INTO `job_definitions` (`name`, `description`, `script`, `created_at`, `updated_at`, `version`) VALUES ('job', 'An description of the job definition.\n\n## Failure Affects\nAffected users, services and/ or business areas.\n\n## Workaround\nChoose one of the following:\n- __Retry__ as soon as possible.\n- Make an urgent call to administrator (Job stays in _Error_ state)\n- Do nothing, and let administrator recover later (Job stays in _Error_ state)\n- Ignore error and _Cancel_ the job (No recovery required)\n\n## Recovery Procedures\nDescribe how to recover from the failure.\n', 'noop:', '2018-11-09 06:48:47', '2018-11-09 06:48:47', 0)
  SQL (0.3ms)  INSERT INTO `admin_assignments` (`user_id`, `job_definition_id`, `created_at`, `updated_at`) VALUES (1, 5, '2018-11-09 06:48:47', '2018-11-09 06:48:47')
  SQL (0.3ms)  INSERT INTO `script_revisions` (`job_definition_id`, `script`, `changed_at`, `created_at`, `updated_at`) VALUES (5, 'noop:', '2018-11-09 06:48:47', '2018-11-09 06:48:47', '2018-11-09 06:48:47')
  Kuroko2::MemoryExpectancy Load (0.3ms)  SELECT  `memory_expectancies`.* FROM `memory_expectancies` WHERE `memory_expectancies`.`job_definition_id` = 5 LIMIT 1
  SQL (0.2ms)  INSERT INTO `memory_expectancies` (`job_definition_id`, `created_at`, `updated_at`) VALUES (5, '2018-11-09 06:48:47', '2018-11-09 06:48:47')
   (0.1ms)  RELEASE SAVEPOINT active_record_1
=> true
[6] pry(main)> d.revisions.size
=> 1
[7] pry(main)> d.revisions[0].errors.full_messages
=> []
eagletmt commented 5 years ago

I got it. This behavior depends on Rails.application.config.active_record.belongs_to_required_by_default flag. https://github.com/cookpad/kuroko2/blob/v0.5.0/spec/dummy/config/initializers/new_framework_defaults.rb#L20 Kuroko2 should be available with the new Rails' defaults.

yohfee commented 5 years ago

Ah, I see. But it may effect other components unintentionally. I try with rails 4 compatibility for the time being.

yukihirop commented 4 years ago

I had a similar problem. As a result of trial and error, we succeeded to work by applying a light monkey patch.

https://github.com/yukihirop/kuroko2

andreped commented 1 year ago

As a result of trial and error, we succeeded to work by applying a light monkey patch.

@yukihirop What exactly was the "light monkey patch"? Can you direct me to the specific commit where this was fixed?


EDIT: I managed to find the necessary edits. After adding these it seems to work. At least I am able to create jobs and they run successfully. For modifications, add both these two scripts: https://github.com/yukihirop/my_kuroko2/commit/d791db92c2d9f56b6957fcc667bd3bcbd7aab465#diff-758ae82c1a1ace4017465eadda2b3401f1f56e2f41c2357ede419ce8b2385ca2 https://github.com/yukihirop/my_kuroko2/commit/d791db92c2d9f56b6957fcc667bd3bcbd7aab465#diff-0dfac06b842a2560bffe8756c2dbfbf6652460a9bdd23d15638c30e5bdfe4578