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

Ghost tables visualization_backups migration STEP 2 #14749

Merged
merged 3 commits into from
Mar 19, 2019

Conversation

oleurud
Copy link
Contributor

@oleurud oleurud commented Mar 15, 2019

Step 2 of the migration related to #14698

Don't merge after #14744 will be deployed (the tests will fail until the deploy too)

Proc.new do
drop_table :visualization_backups
create_table :visualization_backups do
Uuid :id, primary_key: true, default: Sequel.lit('uuid_generate_v4()')

Choose a reason for hiding this comment

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

Use 2 (not 4) spaces for indentation.
Unnecessary spacing detected.
Put one space between the method name and the first argument.

@@ -0,0 +1,26 @@
require 'carto/db/migration_helper'

include Carto::Db::MigrationHelper

Choose a reason for hiding this comment

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

include is used at the top level. Use inside class or module.

Copy link
Contributor

@ethervoid ethervoid left a comment

Choose a reason for hiding this comment

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

Some questions

@oleurud oleurud requested a review from ethervoid March 18, 2019 12:13
@ethervoid
Copy link
Contributor

@oleurud ready to test in staging after the tests are fixed

@oleurud
Copy link
Contributor Author

oleurud commented Mar 18, 2019

The tests won't be green until #14744 will be merged

@oleurud oleurud merged commit dad3702 into master Mar 19, 2019
@oleurud oleurud deleted the ghost_tables_backup_migration_2 branch March 19, 2019 07:52
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.

None yet

4 participants