Danger: Ensure that columns to be renamed/deleted are ignored
requested to merge 389351-add-rubocop-rule-or-danger-check-to-ensure-column-to-be-renamed-is-ignored into master
What does this MR do and why?
To prevent incidents like gitlab-com/gl-infra/production#8264 (closed) and gitlab-com/gl-infra/production#16274 (closed) we must warn that columns being renamed – or deleted, should be ignored on the AR model backed by the respective table.
Example when cleanup_concurrent_column_rename
method is used
No warnings when remove_column
is used in down
Example when remove_column
method is used
Example when cleanup_conversion_of_integer_to_bigint
method is used
How to set up and validate locally
- Locally commit a
migration
usingcleanup_concurrent_column_rename
method
Example
# frozen_string_literal: true
class TestRenamedColumnsMigration < Gitlab::Database::Migration[2.1]
disable_ddl_transaction!
def up
cleanup_concurrent_column_rename :ml_candidates, :iid, :eid
end
def down
undo_cleanup_concurrent_column_rename :ml_candidates, :iid, :eid
end
end
- Locally commit a
post-migration
usingremove_column
method
Example
# frozen_string_literal: true
class TestRemoveColumnsMigration < Gitlab::Database::Migration[2.1]
def up
remove_column :ci_secure_files, :permissions
end
def down
add_column :ci_secure_files, :permissions, :integer, null: false, default: 0, limit: 2
end
end
- run
bin/rake danger_local
- check Markdown output
bin/rake danger_local
Results:
Markdown:
db/migrate/20230913200422_test_renamed_columns_migration.rb#L7
[Dropping](https://docs.gitlab.com/ee/development/database/avoiding_downtime_in_migrations.html#dropping-columns) or [renaming](https://docs.gitlab.com/ee/development/database/avoiding_downtime_in_migrations.html#renaming-columns) columns requires the columns to be
ignored in the model. This step is necessary because Rails caches the columns and re-uses it in various places
across the application. Please, make sure that columns are proper ignored in the model.
db/post_migrate/20230913200616_test_remove_columns_migration.rb#L5
[Dropping](https://docs.gitlab.com/ee/development/database/avoiding_downtime_in_migrations.html#dropping-columns) or [renaming](https://docs.gitlab.com/ee/development/database/avoiding_downtime_in_migrations.html#renaming-columns) columns requires the columns to be
ignored in the model. This step is necessary because Rails caches the columns and re-uses it in various places
across the application. Please, make sure that columns are proper ignored in the model.
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Related to #389351 (closed)
Related to #423659 (closed)
Edited by Leonardo da Rosa