Add migrator.ragtime component #4

ayato-p · 2017-06-15T09:05:27Z

No description provided.

rfkm

I'm really sorry for the delay. I left some comments.

rfkm · 2017-06-19T15:03:13Z

extra/src/zou/migrator/ragtime.clj

+           java.time.format.DateTimeFormatter
+           java.time.LocalDateTime))
+
+(defprotocol IMigrator


I'm recently having second thoughts about making protocols which will be never reused practically (I understand this is based on my code, though..).
How about simply defining functions?

rfkm · 2017-06-19T15:11:47Z

extra/src/zou/migrator/ragtime.clj

+  (let [separator (.getSeparator (FileSystems/getDefault))
+        prefix (str path separator (gen-file-name description))
+        up-sql (io/file (str prefix ".up.sql"))
+        down-sql (io/file (str prefix ".down.sql"))]


IMO, it would be better to pass path to parent paramter of io/file:

(let [prefix (gen-file-name description) up-sql (io/file path (str prefix ".up.sql")) down-sql (io/file path (str prefix ".down.sql"))] ...)

rfkm · 2017-06-19T15:12:16Z

extra/src/zou/migrator/ragtime.clj

+(defn- gen-edn-files [path description]
+  (let [separator (.getSeparator (FileSystems/getDefault))
+        prefix (str path separator (gen-file-name description))
+        edn (io/file (str prefix ".edn"))]


rfkm · 2017-06-19T15:15:47Z

extra/src/zou/migrator/ragtime.clj

+(defn generate* [migrator description]
+  (let [{:keys [migrations-path style]} migrator
+        path (some-> (io/resource migrations-path)
+                     io/file


I guess this will throw an exception when migration-path is an immutable classpath such as URL(= jar).
generate exists only for convenience during development, so the failure itself is ok, but please consider raising a more user-friendly error.

rfkm · 2017-06-19T15:25:59Z

extra/src/zou/migrator/ragtime.clj

+
+  (remigrate [this]
+    (let [{:keys [datastore index migrations]} this
+          last-applied (last (ragtime/applied-migrations datastore index))


I have a question.
What happens when there is a difference between the applied migration and the current one that actually exists on the file system?
Does Ragtime remember the actual content of the applied migrations (not only id)?

Ragtime manage only id.
In that case, Ragtime will fail or just works (it's depends on what the difference between those).
I think rollback also has same issue.

rfkm · 2017-07-08T06:08:52Z

extra/test/zou/migrator/ragtime_test.clj

+              (doseq [f' (.listFiles f)]
+                (func func f')))
+            (io/delete-file f))]
+    (delete-recur delete-recur (io/file h2db-dir)))


I don't remember why such a hacky way is needed (maybe for mutual anonymous recursion?), but I guess the following code should work in this case:

(letfn [(delete-recur [f] (when (.isDirectory f) (doseq [f' (.listFiles f)] (delete-recur f'))) (io/delete-file f))] (delete-recur (io/file h2db-dir)))

rfkm · 2017-07-09T13:03:41Z

extra/src/zou/migrator/ragtime.clj

+                :desc "Rolls back the database"
+                :option-specs [["-n" "--n N" "How many changes to rollback"
+                                :parse-fn #(Long/parseLong %)
+                                :validate [#(re-matches #"^\d+$" %) "Must be integer"]]


IIRC, the validation functions will be applied to parsed values. So, this validation should always fail.
pos? might be a good alternative as a validation function.

rfkm · 2017-07-09T13:11:53Z

extra/src/zou/migrator/ragtime.clj

+      (log/warnf "Migrations directory `%s` does not exist" migrations-path))
+    (case style
+      :edn (gen-edn-files path description)
+      :sql (gen-sql-files path description)


(Optional) I think that's a good use case of mutimethods.

rfkm · 2017-07-09T13:43:29Z

extra/project.clj

+  :plugins [[lein-modules "0.3.11"]]
+  :profiles
+  {:provided {:dependencies [[ragtime "0.7.1"]]}
+   :dev {:resource-paths ["dev-resources"]}})


Is this necessary?

rfkm · 2017-07-09T13:51:46Z

extra/src/zou/migrator/ragtime.clj

+
+  task/Task
+  (task-name [this] :migration)
+  (spec [this] {:desc "Migration tasks"})


I think it is better to use the more concrete name and description like :ragtime and "Ragtime tasks".

rfkm · 2017-09-11T15:46:06Z

extra/src/zou/migrator/ragtime.clj

-  (remigrate [this])
-  (rollback [this opts])
-  (generate [this description]))
+(defn validate-component-state [component]


rfkm · 2017-09-11T15:50:01Z

extra/src/zou/migrator/ragtime.clj

@@ -119,7 +125,7 @@
                :desc "Rolls back the database"
                :option-specs [["-n" "--n N" "How many changes to rollback"
                                :parse-fn #(Long/parseLong %)
-                                :validate [#(re-matches #"^\d+$" %) "Must be integer"]]
+                                :validate [pos? "Must be integer"]]


integer -> positive integer

rfkm · 2017-09-12T16:20:39Z

extra/src/zou/migrator/ragtime.clj

+(defn validate-component-state [component]
+  (let [{:keys [datastore index migrations]} component]
+    (when-not (and datastore index migrations)
+      (throw (ex-info "Ragtime component has not been initialized" {:keys [:datastore :index :migrations]})))


I think this map seems not so useful. It might be better to simply throw IllegalStateException or pass actual data corresponding to the keys to ex-data (if they are an implementation detail, the former (w/ debug log) might be better).

rfkm · 2017-09-20T05:00:43Z

extra/src/zou/migrator/ragtime.clj

+        n (if (nil? n) 1 n)
+        opts (select-keys ragtime-component [:reporter])]
+    (if (and (seq applied)
+             (or (seq id) (integer? n)))


Hmm, IMO, it'd be better to validate arguments separately and raise an error if it fails, rather than just saying "No migrations to roll back".

rfkm · 2017-09-20T05:23:36Z

extra/src/zou/migrator/ragtime.clj

+      (generate-files style (-> resource io/file .getPath) description)
+      (throw (ex-info (format "Migrations directory `%s` %s"
+                              migrations-path
+                              (if resource "not a directory" "does not exist"))


not a directory

It seems like you don't actually check whether it's a directory or not :p

rfkm · 2017-09-20T05:29:32Z

extra/test/zou/migrator/ragtime_test.clj

-(def h2db-file (str h2db-dir "/h2db.sql"))
+(def empty-file-attrs (into-array java.nio.file.attribute.FileAttribute []))
+
+(def h2db-file (Files/createTempFile "h2db-" ".sql" empty-file-attrs))


You can use java.io.File/createTempFile without empty-file-attrs

rfkm · 2017-09-20T12:59:01Z

extra/test/zou/migrator/ragtime_test.clj

+
+(def empty-file-attrs (into-array java.nio.file.attribute.FileAttribute []))
+
+(def h2db-file (Files/createTempFile "h2db-" ".sql" empty-file-attrs))


Btw, why don't you use in-memory mode?

ayato-p requested a review from rfkm June 15, 2017 09:17

Add migrator.ragtime component

b790075

rfkm requested changes Aug 29, 2017

View reviewed changes

fixup! Add migrator.ragtime component

c5d84e8

rfkm requested changes Sep 20, 2017

View reviewed changes

rfkm reviewed Sep 20, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add migrator.ragtime component #4

Add migrator.ragtime component #4

ayato-p commented Jun 15, 2017

rfkm left a comment

rfkm Jun 19, 2017

rfkm Jun 19, 2017

rfkm Jun 19, 2017

rfkm Jun 19, 2017

rfkm Jun 19, 2017

ayato-p Aug 30, 2017

rfkm Jul 8, 2017

rfkm Jul 9, 2017

rfkm Jul 9, 2017

rfkm Jul 9, 2017

rfkm Jul 9, 2017

rfkm Sep 11, 2017

rfkm Sep 11, 2017

rfkm Sep 12, 2017

rfkm Sep 20, 2017

rfkm Sep 20, 2017

rfkm Sep 20, 2017

rfkm Sep 20, 2017


		(def empty-file-attrs (into-array java.nio.file.attribute.FileAttribute []))

		(def h2db-file (Files/createTempFile "h2db-" ".sql" empty-file-attrs))

Add migrator.ragtime component #4

Are you sure you want to change the base?

Add migrator.ragtime component #4

Conversation

ayato-p commented Jun 15, 2017

rfkm 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

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