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

Added strong_migrations gem and initializer #3151

Merged
merged 3 commits into from Jul 24, 2019

Conversation

@zwolf
Copy link
Member

commented Jul 12, 2019

Initializer tells the gem to ignore all old migrations, so we're just checking from here forward and only in test/dev envs.

For reference: https://github.com/ankane/strong_migrations

Review checklist

  • First, the most important one: is this PR small enough that you can actually review it? Feel free to just reject a branch if the changes are hard to review due to the length of the diff.
  • If there are any migrations, will they the previous version of the app work correctly after they've been run (e.g. the don't remove columns still known about by ActiveRecord).
  • If anything changed with regards to the public API, are those changes also documented in the apiary.apib file?
  • Are all the changes covered by tests? Think about any possible edge cases that might be left untested.
@@ -0,0 +1 @@
StrongMigrations.start_after = 20190624094308

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jul 12, 2019

Style/NumericLiterals: Use underscores(_) as decimal mark and separate every 3 digits with them.

Copy link
Member

left a comment

LGTM - What was the reasoning to only have these in the dev / test envs? It might be useful to have these guards on staging / prod as well, especially guards like, https://github.com/ankane/strong_migrations#dangerous-tasks

Are there complications / overhead to have these in the non-dev / test envs?

By the look of the current setup, this won't run the checks on our travis CI, as that currently runs the travis config and then rake db:setup,

- bundle exec rake configure:travis db:setup

Ideally this would run before CI and ensure we haven't circumvented the checks manually via manual code check in to the schema in structure.sql

@github-actions github-actions bot added the approved label Jul 15, 2019
@zwolf

This comment has been minimized.

Copy link
Member Author

commented Jul 15, 2019

My assumption was that if you include a migration in a PR, you would have already run that migration locally while writing specs and seen any potential issues it raised, then either included a circumvention (and an explanation) or fixed it before submitting.

This isn't a benchmarking tool and any errors/suggestions it raises in CI it would raise locally first. But by the same token I don't see any harm in it. if you think there's utility in letting it go nuts, I'll put it in production, too.

@camallen

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

My assumption was that if you include a migration in a PR, you would have already run that migration locally while writing specs and seen any potential issues it raised, then either included a circumvention (and an explanation) or fixed it before submitting.

This isn't a benchmarking tool and any errors/suggestions it raises in CI it would raise locally first. But by the same token I don't see any harm in it. if you think there's utility in letting it go nuts, I'll put it in production, too.

There is nothing to stop someone circumventing this (small risk) and the dev team assuming that the checks ran but really they didn't. I.e. we stop reviewing that migration code risk but rely on an automated agent that we have no guarantee it ran. This is my fear that we get lazy on this...hence I was wondering about CI tooling.

@zwolf zwolf merged commit 056ae98 into zooniverse:master Jul 24, 2019
3 checks passed
3 checks passed
Hound 1 violation found.
continuous-integration/jenkins/pr-merge This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zwolf zwolf deleted the zwolf:safe-migrations branch Jul 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.