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

Allow configuration to skip the execute_inline! call #137

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

nevinera
Copy link

@nevinera nevinera commented Dec 6, 2022

Purpose

Since updating to lhm-shopify, we had a recurring issue where people would write a migration using LHM (we have it in our generator templates), but fill it with normal calls to migration methods like add_column(:tablename, :colname, :string).

Before the upgrade, these calls would get caught immediately, because the schema file wouldn't get the intended changes, and any staging stack based on that branch would not have the column present. But after the upgrade, these calls worked fine locally, and since our staging stacks are based on the schema file (not by running migrations from scratch), we only end up catching these problems in production.

When we realized what was happening, we tracked it to here - the explanations on the change make total sense, but in our case we'd much rather exclude development from inlining, so this problem will get caught locally. (You should probably consider dropping the development? condition entirely - I expect other people are getting surprised here occasionally as well).

Implementation

  • Fix a typo I noticed in the docs while setting up for specs (bundle exec rake specs to run all specs)
  • Update the README to note the new configuration option, without going into much detail about why you'd care
  • Add Lhm.disallow_inline! and Lhm.inline_allowed? to the base module for configuration purposes
  • Add an extra check against Lhm.inline_allowed? to the Railtie. Didn't see any tests on this thing, but I'm happy to go learn how to make one if you'd like.

Verification

Confirmed locally that with the setting not supplied migrations run inline, and with Lhm.disallow_inline! called, they do not.

Note

If you'd rather drop the 'auto-inline in development' part, that's a smaller change, and I'd be happy to update the PR :-)

@nevinera nevinera marked this pull request as ready for review December 6, 2022 19:50
@nevinera nevinera marked this pull request as draft December 6, 2022 19:55
@nevinera nevinera marked this pull request as draft December 6, 2022 19:55
@nevinera nevinera force-pushed the allow-env-to-stop-inline branch from 5ddc99f to 8accd92 Compare December 6, 2022 20:03
@nevinera nevinera marked this pull request as ready for review December 6, 2022 20:04
@nevinera nevinera force-pushed the allow-env-to-stop-inline branch from 8accd92 to 39a6fa9 Compare December 6, 2022 20:23
**Note:** When developing locally or running tests, LHM runs 'inline' by default, meaning that the
extra table is not used and the original table is not replaced. If this behavior is not desirable
for your project, you can disable it by setting `Lhm.disallow_inline!` in the appropriate
environment file or initializer.
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if you think more explanation (of why it does that, or why you might want to opt out of it) is warranted here.

@nevinera
Copy link
Author

Any chance of a review? I'd be happy to make other changes, but this repo feels somewhat forgotten :-/

Copy link

@fmichaut-diff fmichaut-diff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has definitively tripped us a dozen times, but for a different reason

LHM does not support virtual columns / foreign keys, and if we forget to temporarily remove the foreign keys at the beginning of the migration, the migration will pass on Dev, but fail on Prod.

We would also like to be able to not run LHM inline on Dev because of theses difference of behavior, and catch errors early instead of catching them when they reach Prod...

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.

2 participants