soundcloud / lhm

Online MySQL schema migrations
BSD 3-Clause "New" or "Revised" License
1.83k stars 190 forks source link

Mysql 8 no display attribute on fields #162

Open montdidier opened 3 years ago

montdidier commented 3 years ago

Just thought I should feed back this to the original project.

For Mysql 8, display attributes on fields is deprecated, it can be fixed easily enough. I would setup a PR myself but already have one to the Shopify fork of LHM which we have switched to.

See here https://dev.mysql.com/worklog/task/?id=13127

PR to Shopify fork.

https://github.com/Shopify/lhm/pull/101

diff --git a/lib/lhm/table.rb b/lib/lhm/table.rb
index c913169..a2663bd 100644
--- a/lib/lhm/table.rb
+++ b/lib/lhm/table.rb
@@ -18,7 +18,7 @@ def initialize(name, pk = 'id', ddl = nil)

     def satisfies_id_column_requirement?
       !!((id = columns['id']) &&
-        id[:type] =~ /(bigint|int)\(\d+\)/)
+        id[:type] =~ /(bigint|int)(\(\d+\))?/)
     end

     def destination_name
diff --git a/spec/unit/table_spec.rb b/spec/unit/table_spec.rb
index c75181f..d5ad1f9 100644
--- a/spec/unit/table_spec.rb
+++ b/spec/unit/table_spec.rb
@@ -32,6 +32,12 @@ def set_columns(table, columns)
       @table.satisfies_id_column_requirement?.must_equal true
     end

+    it 'should be satisifed if display attributes are not present (deprecated in mysql 8)' do
+      @table = Lhm::Table.new('table', 'id')
+      set_columns(@table, { 'id' => { :type => 'int' } })
+      @table.satisfies_id_column_requirement?.must_equal true
+    end
+
     it 'should not be satisfied if id is not numeric' do
       @table = Lhm::Table.new('table', 'id')
       set_columns(@table, { 'id' => { :type => 'varchar(255)' } })
montdidier commented 3 years ago

The real question is if this particular repository is even supported anymore.