Skip to content

Danger: Ensure that columns to be renamed/deleted are ignored

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

  1. Locally commit a migration using cleanup_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
  1. Locally commit a post-migration using remove_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
  1. run bin/rake danger_local
  2. 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.

Related to #389351 (closed)

Related to #423659 (closed)

Edited by Leonardo da Rosa

Merge request reports

Loading