search

lfittl / activerecord-clean-db-structure

Automatic cleanup for the Rails db/structure.sql file (ActiveRecord/PostgreSQL)
BSD 3-Clause "New" or "Revised" License
146 stars 36 forks source link

Fix incorrect primary structure.sql filename in the case of multi-database setup #25

Closed RKushnir closed 2 months ago

RKushnir commented 4 years ago

Hi there, we just introduced a secondary database to our Rails 6 app and were surprised to see db:migrate fail with this error:

Errno::ENOENT: No such file or directory @ rb_sysopen - db/primary_structure.sql
…/activerecord-clean-db-structure/lib/activerecord-clean-db-structure/tasks/clean_db_structure.rake:25:in `read'

Here's a little fix to get past this bummer.

woodhull commented 2 months ago

We just ran into this same issue.

andyatkinson commented 2 months ago

@lfittl My only hesitation was I didn't see any mentions of confirmations this fixed the issue and didn't cause any regressions in this PR. I could do some manual testing using a Rails app with (a) a single DB config (b) a multi-DB config where the main database is named "primary" and (c) where it has a different name and report back.

I have my book sample app set up to (manually) switch between single and multi-DB configs, so I could set some time aside to test that.

However, if we're ok with the changes as is, or others can vouch for this, then I'm with merging. I'm happy to cut a new small release either way too.

Thanks.

woodhull commented 2 months ago

I implemented this change today. It seems to work fine in the multi DB scenario.

On Fri, Sep 6, 2024, 12:56 PM Andrew Atkinson @.***> wrote:

@lfittl https://github.com/lfittl My only hesitation was I didn't see any mentions of confirmations this fixed the issue and didn't cause any regressions in this PR. I could do some manual testing using a Rails app with (a) a single DB config (b) a multi-DB config where the main database is named "primary" and (c) where it has a different name and report back.

I have my book sample app set up to (manually) switch between single and multi-DB configs, so I could set some time aside to test that.

However, if we're ok with the changes as is, or others can vouch for this, then I'm with merging. I'm happy to cut a new small release either way too.

Thanks.

— Reply to this email directly, view it on GitHub https://github.com/lfittl/activerecord-clean-db-structure/pull/25#issuecomment-2334461398, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAACIPT2PFTZEYD2M4NOXT3ZVHNFLAVCNFSM6AAAAABNLPQC3SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMZUGQ3DCMZZHA . You are receiving this because you commented.Message ID: @.***>

lfittl commented 2 months ago

Per @woodhull's comment, I'll go ahead and merge.

lfittl commented 2 months ago

Hmm. Interesting. It looks like GitHub won't let me merge because the original repo was deleted. Making a new PR.

lfittl commented 2 months ago

Merged via #44.

@andyatkinson I think another release might be good, though we could wait for #43 to be done first.

RKushnir commented 2 months ago

@lfittl Thanks, it's pleasant to see it merged :green_heart:

After this got my attention last week, I was thinking about revising the PR to also support the multi-db case where there's no explicit "primary" config. But, I believe, in existing form the PR already covers most use cases.