search

lastland / scala-forklift

Type-safe data migration tool for Slick, Git and beyond.
Other
188 stars 31 forks source link

Copy the migration file instead of creating a symbolic link #35

Closed softinio closed 7 years ago

softinio commented 7 years ago

This PR should fix #31 and #28

It is better to copy the migration file over than to create a symbolic link as migrations may be generated in one place but used and deployed elsewhere.

The link path when I generated the migration on my laptop got saved to source control with my laptop home directory in its path. This is wrong of course.

CLAassistant commented 7 years ago

CLA assistant check
All committers have signed the CLA.

lastland commented 7 years ago

Hi @softinio, thanks for the contribution!

It is better to copy the migration file over than to create a symbolic link as migrations may be generated in one place but used and deployed elsewhere.

In the meantime, symbolic links have the advantage that if you decide to change the migration file during development, you can do that on the linked file and your editor should usually be able to make changes to the actual file (that's at least true for Emacs).

Probably a better idea is to have two options for users to choose from? What do you think?

softinio commented 7 years ago

@lastland giving user choice is a good idea. Will work on that.

softinio commented 7 years ago

@lastland Change to make symbolic link or copying migration an option for the user. Over to you to review. Thanks.

softinio commented 7 years ago

wait I made a mistake

softinio commented 7 years ago

@lastland can you take a look ? Thanks.

lastland commented 7 years ago

@softinio Thank you! I'm working on something else right now. I will take a look at it tonight!

softinio commented 7 years ago

@lastland ok no problem. Is there anything to setup to run tests?

lastland commented 7 years ago

I think you would need setup databases in addition to the usual Scala and sbt environment. If you want to run the tests, just run sbt test and see which tests fail. And if the failed tests are related to a particular database, you probably haven't set it up correctly.

I don't remember why the CI for PR is not working...

lastland commented 7 years ago

Oh, I forgot to mention that you need to publishLocal before running the tests because the tests actually run sbt on an example project which depends on the library, and it will only find the latest library if you do publishLocal. The complete command can be found in the CI configuration: https://github.com/lastland/scala-forklift/blob/develop/circle.yml

lastland commented 7 years ago

This looks good to me. Thanks again for the contribution!

softinio commented 7 years ago

@lastland your welcome. Glad I helped. Quick question when are you likely to publish this to maven as a new version to 0.3.0 ?

lastland commented 7 years ago

@softinio Oh thanks for reminding me of that! I should do that very soon.

lastland commented 7 years ago

@softinio I have released v0.3.1, but it may take a few hours to be present at Maven Central.

smedberg commented 7 years ago

@lastland Hey, do you intend to release a version of v0.3.1 (with this fix) which is compatible with Scala 2.11? The "how to" seems to say that if you're on Scala 2.11 and Slick 3.2, you should use Forklift 0.2.2. Thanks!

lastland commented 7 years ago

@smedberg Hi, I didn't release v0.3.1 with Scala 2.11 because I haven't done a thorough testing on Scala 2.11. Now that you have asked, I will see if I can get something done tonight. :)

smedberg commented 7 years ago

@lastland Thank you so much!

lastland commented 7 years ago

I should also document this change in README! I leave a comment here to remind myself.

lastland commented 7 years ago

@smedberg Published. Again, it would take some time to be present on MVN central.

smedberg commented 7 years ago

Thanks!

On Tue, Aug 22, 2017 at 7:43 PM, Li Yao notifications@github.com wrote:

@smedberg https://github.com/smedberg Published. Again, it would take some time to be present on MVN central.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/lastland/scala-forklift/pull/35#issuecomment-324205455, or mute the thread https://github.com/notifications/unsubscribe-auth/AGKg88IkE6NLu8KyxybF6CCAhgaEoKRiks5sa5G8gaJpZM4OrYck .