Add script history #110

takonomura · 2018-04-06T03:26:10Z

Add page to show script revisions.

riseshia · 2018-04-06T03:29:07Z

app/models/kuroko2/job_definition.rb

@@ -30,6 +30,7 @@ module PreventMultiStatus
  has_many :job_suspend_schedules, dependent: :delete_all
  has_many :job_definition_tags
  has_many :tags, through: :job_definition_tags
+  has_many :revisions, -> { order(:id).reverse_order }, class_name: 'JobDefinitionRevision', dependent: :destroy


order(id: :desc) ?

eisuke

I think we will want revision's permalink.
Are there plans to add it? (It is good even after this pull request.)

eisuke · 2018-04-09T11:52:47Z

app/models/kuroko2/job_definition.rb

@@ -100,6 +101,12 @@ def create_instance(script: nil, launched_by:, token: nil )
    job_instances.create!(script: script, log_message: message)
  end

+  def record_revision(user: nil)
+    unless revisions.first.try(:script) == script
+      Kuroko2::JobDefinitionRevision.create(job_definition: self, script: script, user: user, changed_at: Time.now)


Kuroko2 needs to follow Rails time zone configuration. Please use Time.zone.now.

eisuke · 2018-04-09T12:03:43Z

app/models/kuroko2/job_definition.rb

@@ -30,6 +30,7 @@ module PreventMultiStatus
  has_many :job_suspend_schedules, dependent: :delete_all
  has_many :job_definition_tags
  has_many :tags, through: :job_definition_tags
+  has_many :revisions, -> { order(id: :desc) }, class_name: 'JobDefinitionRevision', dependent: :destroy


The JobDefinition model already has a version column. It may be confusing.
Possible options are as follows, I think.

Remove JobDefinition's revision column.

A JobDefinitionRevision has JobDefinition's revision column.

I think No.1 is better. What do you think?

eisuke · 2018-04-09T12:18:01Z

app/controllers/kuroko2/api/job_definitions_controller.rb

@@ -16,6 +16,7 @@ def create_resource
        user.stars.create(job_definition: definition) if user.google_account?
      end

+      definition.record_revision


You may do it in JobDefinition's after_save.

riseshia · 2018-04-09T12:39:56Z

reviewable pr? 🤔

eagletmt · 2018-04-12T08:09:46Z

db/migrate/033_create_job_definition_revisions.rb

+      t.belongs_to :job_definition, foreign_key: true, null: false
+      t.text :script, null: false
+      t.belongs_to :user, foreign_key: true, null: true
+      t.datetime :changed_at, null: true


When does changed_at become null?

Job definitions created before this changes have revision with null changed_at.

kuroko2/db/migrate/033_create_job_definition_revisions.rb

Line 18 in 74841d6

Kuroko2::JobDefinitionRevision.create(job_definition: definition, script: definition.script)

Using updated_at as first changed_at seems reasonable to me, how do you think?

Using updated_at as first changed_at

Sorry, I don't understand. What do you mean?

I mean

Kuroko2::JobDefinitionRevision.create(job_definition: definition, script: definition.script)

to

Kuroko2::JobDefinitionRevision.create(job_definition: definition, script: definition.script, changed_at: definition.updated_at)

I changed in 3d7e119.

eagletmt · 2018-04-12T08:11:22Z

app/models/kuroko2/job_definition.rb

@@ -19,8 +19,6 @@ module PreventMultiStatus
    PreventMultiStatus::ERROR => [Kuroko2::Token::FAILURE, Kuroko2::Token::CRITICAL],
  }

-  self.locking_column = :version


Why did you disable optimistic locking? http://api.rubyonrails.org/classes/ActiveRecord/Locking/Optimistic.html

Because version column has removed.

kuroko2/db/migrate/033_create_job_definition_revisions.rb

Line 3 in 74841d6

remove_column :job_definitions, :version

Why did you remove version column...?

#110 (comment)

Oops... I forgot that JobDefinition using optimistic locking. Please revert the commit 🙏

I reverted the commit (4570e60)

eagletmt · 2018-04-12T08:13:34Z

app/controllers/kuroko2/job_definitions_controller.rb

@@ -50,6 +50,7 @@ def create

    if @definition.save
      current_user.stars.create(job_definition: @definition)
+      @definition.revisions.first.update(user: current_user)


This should be executed in the same transaction with @definition.save, I think.
Since revision requires user_id, running record_revision in after_save hook looks bad idea to me.

So should I revert 74841d6?
#110 (comment) @eisuke

This should be executed in the same transaction with @definition.save

I agree. But I'd like to avoid writing definition.record_revision in some controllers.
How about adding the #save_and_record_revision(attributes, edited_user: nil) method to JobDefinition model ?

I changed in debd0db.

eagletmt · 2018-04-12T08:15:12Z

db/migrate/033_create_job_definition_revisions.rb

+    end
+
+    Kuroko2::JobDefinition.all.each do |definition|
+      Kuroko2::JobDefinitionRevision.create(job_definition: definition, script: definition.script)


This migration should not be run in rails db:rollback

Thanks. I fixed in 8cc9ec4.

riseshia

IMO) This resource will be created when JobDefinition's script changed, hence, JobDefinitionRevision may be not a good name.
How about just ScriptRevision?

riseshia · 2018-04-25T04:33:03Z

app/models/kuroko2/job_definition.rb

@@ -100,6 +101,18 @@ def create_instance(script: nil, launched_by:, token: nil )
    job_instances.create!(script: script, log_message: message)
  end

+  def save_and_record_revision(edited_user: nil)


This should be executed in the same transaction with @definition.save, I think.
Since revision requires user_id, running record_revision in after_save hook looks bad idea to me.

I think @eagletmt means not only make it in one method, but execute in the real transaction, doesn't it?

I fixed in 1066340.

riseshia · 2018-04-25T04:40:08Z

app/views/kuroko2/job_definition_revisions/index.html.slim

@@ -0,0 +1,24 @@
+- content_for :title, "##{@definition.id} #{@definition.name} « Job Definitions"


Definitions -> Definition Revisions?

I fixed in bc22e19.

riseshia · 2018-04-25T04:43:25Z

app/views/kuroko2/job_definition_revisions/index.html.slim

+
+style= Diffy::CSS
+
+- @revisions.each.with_index do |revision, i|


(maybe) Use each_cons(2) instead of this.
then html_diff doesn't need to allow nil case, is it right?

If using each_cons(2), oldest revision will not displayed.

Oh, my comment was not proper. I should say why you want to show the oldest revision. (weak opinion)
It's better to use each_with_index(1) if you let it leave.

Showing the oldest revision explains initial script and who created the definition. I think this is helpful.

riseshia · 2018-04-25T04:51:00Z

spec/dummy/db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.

-ActiveRecord::Schema.define(version: 32) do
+ActiveRecord::Schema.define(version: 33) do


👀 many tables seems to be deleted from schema, is this intended?

Deleted tables were duplicated in schema.

riseshia · 2018-04-25T04:55:52Z

db/migrate/033_create_job_definition_revisions.rb

+      t.belongs_to :job_definition, foreign_key: true, null: false
+      t.text :script, null: false
+      t.belongs_to :user, foreign_key: true, null: true
+      t.datetime :changed_at, null: true


Using updated_at as first changed_at seems reasonable to me, how do you think?

riseshia · 2018-04-25T04:57:03Z

spec/controllers/job_definitions_controller_spec.rb

+    describe 'changes script' do
+      let(:script) { "noop:\nnoop:\n" }
+      it do
+      expect(response).to redirect_to(assigns(:definition))


riseshia · 2018-04-25T04:57:11Z

spec/controllers/job_definitions_controller_spec.rb

+      expect(assigns(:definition).revisions.size).to eq 2
+      expect(assigns(:definition).revisions.first.script).to eq script
+      expect(assigns(:definition).revisions.first.user).to eq controller.current_user
+


empty line 👀

Thank you. I fixed in 12e87fa.

riseshia · 2018-04-27T06:47:50Z

app/models/kuroko2/job_definition.rb

+    save
+  end
+
+  def update_and_record_revision(attributes, edited_user: nil)


Is this really worked in transaction? I don't think so 🤔
L105 seems to work well because job_definition is not saved when script revision assignment occurs.

irb(main):008:0> jd = JobDefinition.last JobDefinition Load (0.4ms) SELECT `job_definitions`.* FROM `job_definitions` ORDER BY `job_definitions`.`id` DESC LIMIT 1 => #<JobDefinition id: 1, script: "a", created_at: "2018-04-27 06:42:42", updated_at: "2018-04-27 06:42:42"> irb(main):009:0> jd.assign_attributes(script: 'updated') => nil irb(main):010:0> jd.script_revisions << ScriptRevision.new(script: 'updated') (0.2ms) BEGIN ScriptRevision Create (6.1ms) INSERT INTO `script_revisions` (`script`, `job_definition_id`, `created_at`, `updated_at`) VALUES ('updated', 1, '2018-04-27 06:47:05', '2018-04-27 06:47:05') (0.5ms) COMMIT ScriptRevision Load (26.8ms) SELECT `script_revisions`.* FROM `script_revisions` WHERE `script_revisions`.`job_definition_id` = 1 LIMIT 11 => #<ActiveRecord::Associations::CollectionProxy [#<ScriptRevision id: 1, script: "ab", job_definition_id: 1, created_at: "2018-04-27 06:42:42", updated_at: "2018-04-27 06:42:42">, #<ScriptRevision id: 2, script: "ccccc", job_definition_id: 1, created_at: "2018-04-27 06:43:15", updated_at: "2018-04-27 06:43:15">, #<ScriptRevision id: 3, script: "ccbsadf", job_definition_id: 1, created_at: "2018-04-27 06:43:34", updated_at: "2018-04-27 06:43:34">, #<ScriptRevision id: 4, script: "updated", job_definition_id: 1, created_at: "2018-04-27 06:47:05", updated_at: "2018-04-27 06:47:05">]> irb(main):011:0> jd.save (0.2ms) BEGIN JobDefinition Update (6.8ms) UPDATE `job_definitions` SET `script` = 'updated', `updated_at` = '2018-04-27 06:47:18' WHERE `job_definitions`.`id` = 1 (0.8ms) COMMIT => true

I fixed in dbb501e.

riseshia

LGTM XD

There is the problem that tables has different type of primary key from its installed: - installed before 0.2.0: use unsigned int for primary key - installed after 0.2.0: use signed int for primary key This problem was maden by fa75b20. This makes hard to keep consistency of code / infra. For instance, we need to care about 2 types of primary when adding foreign key. Actually, this problem already occured at cookpad#110 To solve the problem, this patch proposes to alter these primary key to bigint, which is next default type of rails.

There is the problem that some tables has different type of primary key from its installed: - before 0.2.0: using unsigned int for primary key - after 0.2.0: using signed int for primary key This problem was made by fa75b20. This makes hard to keep consistency of code / infra. For instance, we need to care about 2 types of primary when adding foreign key. Actually, this is already blocker of cookpad#110 To solve the problem, this patch proposes to alter these primary key to bigint, which is next default type of rails.

riseshia · 2018-05-21T00:58:54Z

#117 is merged, could you update this pr? :)

takonomura · 2018-05-23T00:54:07Z

Rebased

riseshia · 2018-05-23T02:03:57Z

👍 Give me some more time to merge

riseshia · 2018-05-28T02:12:29Z

Thanks!!! XD

riseshia reviewed Apr 6, 2018

View reviewed changes

eisuke reviewed Apr 9, 2018

View reviewed changes

takonomura force-pushed the feature/script-history branch 3 times, most recently from 59a28c2 to 74841d6 Compare April 11, 2018 04:13

eagletmt reviewed Apr 12, 2018

View reviewed changes

riseshia reviewed Apr 25, 2018

View reviewed changes

riseshia reviewed Apr 27, 2018

View reviewed changes

riseshia approved these changes Apr 27, 2018

View reviewed changes

riseshia mentioned this pull request May 17, 2018

Alter {users,job_definitions}.id to bigint #117

Merged

Add script history

c6130ed

takonomura force-pushed the feature/script-history branch from dbb501e to c6130ed Compare May 23, 2018 00:50

riseshia merged commit 43ecd97 into cookpad:master May 28, 2018

takonomura deleted the feature/script-history branch May 30, 2018 00:44

		@@ -0,0 +1,24 @@
		- content_for :title, "##{@definition.id} #{@definition.name} « Job Definitions"


		style= Diffy::CSS

		- @revisions.each.with_index do \|revision, i\|

Add script history #110

Add script history #110

Conversation

takonomura commented Apr 6, 2018

Choose a reason for hiding this comment

eisuke left a comment

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

riseshia commented Apr 9, 2018

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riseshia left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takonomura Apr 25, 2018 • edited Loading

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

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

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

riseshia left a comment • edited Loading

Choose a reason for hiding this comment

riseshia commented May 21, 2018

takonomura commented May 23, 2018

riseshia commented May 23, 2018

riseshia commented May 28, 2018

takonomura Apr 25, 2018 •

edited

Loading

riseshia left a comment •

edited

Loading