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

Introduce AllCops: MigratedSchemaVersion config #1383

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

koic
Copy link
Member

@koic koic commented Nov 8, 2024

This PR introduces AllCops: MigratedSchemaVersion config.

By specifying MigratedSchemaVersion option, migration files that have been migrated can be ignored. When MigratedSchemaVersion: '20241231000000' is set. Migration files lower than or equal to '20250101000000' will be ignored. For example, this is the timestamp in db/migrate/20250101000000_create_articles.rb.

AllCops
  MigratedSchemaVersion: 20250101000000

This prevents inspecting schema settings for already applied migration files and avoids having different database schemas depending on when bin/rails db:migrate is executed.


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.
  • If this is a new cop, consider making a corresponding update to the Rails Style Guide.

This PR introduces `AllCops: MigratedSchemaVersion` config.

By specifying `MigratedSchemaVersion` option, migration files that have been migrated can be ignored.
When `MigratedSchemaVersion: '20241231000000'` is set. Migration files lower than or equal to '20250101000000' will be ignored.
For example, this is the timestamp in db/migrate/20250101000000_create_articles.rb.

```yaml
AllCops
  MigratedSchemaVersion: 20250101000000
```

This prevents inspecting schema settings for already applied migration files and avoids having different database schemas
depending on when `bin/rails db:migrate` is executed.
Copy link
Contributor

@Earlopain Earlopain left a comment

Choose a reason for hiding this comment

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

I like this, much nicer than having to configure it for each cop individually. This makes sense I think, you never want any of these cop to run for already run migrations and potentially change them. cc @fatkodima

### `AllCops: MigratedSchemaVersion`

By specifying `MigratedSchemaVersion` option, migration files that have been migrated can be ignored.
When `MigratedSchemaVersion: '20241231000000'` is set. Migration files lower than or equal to '20250101000000' will be ignored.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
When `MigratedSchemaVersion: '20241231000000'` is set. Migration files lower than or equal to '20250101000000' will be ignored.
When `MigratedSchemaVersion: '20250101000000'` is set. Migration files lower than or equal to '20250101000000' will be ignored.

Comment on lines +57 to +68
By specifying `MigratedSchemaVersion` option, migration files that have been migrated can be ignored.
When `MigratedSchemaVersion: '20241231000000'` is set. Migration files lower than or equal to '20250101000000' will be ignored.
For example, this is the timestamp in db/migrate/20250101000000_create_articles.rb.

[source,yaml]
----
AllCops
MigratedSchemaVersion: '20250101000000'
----

This prevents inspecting schema settings for already applied migration files and avoids having different database schemas
depending on when `bin/rails db:migrate` is executed.
Copy link
Contributor

@Earlopain Earlopain Nov 11, 2024

Choose a reason for hiding this comment

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

I rewrote this section to add a bit more details. There is one error here where you write 20241231000000 that should be changed to 20250101000000 regardless.

Suggested change
By specifying `MigratedSchemaVersion` option, migration files that have been migrated can be ignored.
When `MigratedSchemaVersion: '20241231000000'` is set. Migration files lower than or equal to '20250101000000' will be ignored.
For example, this is the timestamp in db/migrate/20250101000000_create_articles.rb.
[source,yaml]
----
AllCops
MigratedSchemaVersion: '20250101000000'
----
This prevents inspecting schema settings for already applied migration files and avoids having different database schemas
depending on when `bin/rails db:migrate` is executed.
By specifying the `MigratedSchemaVersion` option, migration files that have already been run can be ignored.
When `MigratedSchemaVersion: '20250101000000'` is set, migration files lower than or equal to '20250101000000' will be ignored.
For example, to ignore `db/migrate/20250101000000_create_articles.rb` and earlier migrations you would configure it the following way:
[source,yaml]
----
AllCops
MigratedSchemaVersion: '20250101000000'
----
This prevents inspecting schema settings for already applied migration files. Changing already applied migrations should be avoided because it can lead to the schema getting out of sync between your local copy and what it actually is in production,
depending on when `bin/rails db:migrate` was executed.
If you want to modify your schema to comply with the cops, you should instead create new migrations.

@Earlopain
Copy link
Contributor

Hm, would it be possible to add a hint for MigratedSchemaVersion to the cop message automatically? I think that would be a nice little improvement. If a new cop gets added that looks at migrations there will be many new offenses that should not be fixed.

Configure MigratedSchemaVersion if this migration has already been run.

Or something like that.

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