Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check db/migrate/ instead of schema.rb #177

Merged
merged 1 commit into from
Oct 10, 2014
Merged

Conversation

chuckd
Copy link
Contributor

@chuckd chuckd commented Mar 31, 2014

This seems to me the correct way to check for migrations: diff the db/migrate/ folders.

It avoids the problems with schema_format :sql entirely. No need to try and read the schema_format setting or check either/or schema.rb/structure.sql. Fixes #57, #59 and #74.

This avoids the problems with schema_format :sql. Fixes mina-deploy#57, mina-deploy#59 and mina-deploy#74.
@nathanvda
Copy link

On a side-note: rake db:migrate knows exactly how to check if a migration needs to be executed or not. Sooooooo I am not sure, but it seems a bit silly to be checking a complete folder for changes, if rails has already solved this.

If we check only a single file, I can get there is a possible performance enhancement (just skip if you know nothing has changed). But this is what caused all the problems in the first place :)

I am also using schema_format :sql so I am definitely interested in a fix. Just thinking out loud.

@chuckd
Copy link
Contributor Author

chuckd commented May 12, 2014

@nathanvda the main gain is not having to boot rails, against which a recursive diff is nothing: for what it's worth, a folder diff here (with 94 migrations) takes 17 milliseconds, vs 3.7 seconds for the no-op migrate.

@chuckd
Copy link
Contributor Author

chuckd commented May 12, 2014

@rstacruz any chance of a decision on this one? I just realised I've been running this accidentally for the past month through bodgy direct edit of the gem. It's been working fine for me...

@Kriechi
Copy link

Kriechi commented Aug 3, 2014

I think this is better then the current solution. 👍 for merging this!

Although, as mentioned, I'd probably spend 4 seconds doing it the correct way - spinning up Rails and let the Rails people decide if we should migrate something.

@Kriechi
Copy link

Kriechi commented Sep 3, 2014

Additional note:
capistrano-rails now also uses a db/migrate/ folder diff to migrate on demand.
https://github.com/capistrano/rails/blob/master/lib/capistrano/tasks/migrations.rake#L10

gabskoro added a commit that referenced this pull request Oct 10, 2014
Check db/migrate/ instead of schema.rb
@gabskoro gabskoro merged commit 882da9c into mina-deploy:master Oct 10, 2014
@gabskoro
Copy link
Member

I agree, it's better to check the migrate folder :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants