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

Police migrations #13076

Merged
merged 6 commits into from
Jan 21, 2025
Merged

Police migrations #13076

merged 6 commits into from
Jan 21, 2025

Conversation

mkllnk
Copy link
Member

@mkllnk mkllnk commented Jan 15, 2025

What? Why?

I noticed in recent pull requests that new migrations were not following our style guide and CI still passed because we are ignoring all database related files.

While we don't want to change old migrations, new migrations should adhere to our current coding standards. The new configuration will enforce that.

One downside is that new migrations will grow old over time and new cops will still apply unless they support the new MigratedSchemaVersion option. So we may want to update the configuration periodically, like once a year, if it becomes inconvenient.

It's not obvious, but the MigratedSchemaVersion option applies only to some cops. Most cops still check all old migrations as well. That's why I added patterns to ignore all migrations before 2025.

What should we test?

  • Specs only.

Release notes

Changelog Category (reviewers may add a label for the release notes):

  • User facing changes
  • API changes (V0, V1, DFC or Webhook)
  • Technical changes only
  • Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

@mkllnk mkllnk added the technical changes only These pull requests do not contain user facing changes and are grouped in release notes label Jan 15, 2025
@mkllnk mkllnk self-assigned this Jan 15, 2025
@mkllnk mkllnk marked this pull request as ready for review January 15, 2025 01:46
Copy link
Member

@dacook dacook left a comment

Choose a reason for hiding this comment

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

Great work!
But does this affect the usability of the users script?

db/seeds.rb Outdated
Spree::Role.where(:name => "user").first_or_create
Rails.logger.debug "[db:seed] Seeding Roles"
Spree::Role.where(name: "admin").first_or_create
Spree::Role.where(name: "user").first_or_create
Copy link
Member

Choose a reason for hiding this comment

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

I thought we got rid of "user", or did we just talk about it..

Copy link
Member Author

Choose a reason for hiding this comment

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

The user role is unused at the moment. And we talked about getting rid of roles. But it's still there.

@mkllnk mkllnk requested a review from dacook January 15, 2025 23:48
Copy link
Collaborator

@rioug rioug left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@rioug
Copy link
Collaborator

rioug commented Jan 19, 2025

@mkllnk I'll leave it to you to fix the conflict

@mkllnk mkllnk force-pushed the police-migrations branch from c375c9a to fcc31ff Compare January 21, 2025 00:21
@mkllnk mkllnk merged commit a1df61c into openfoodfoundation:master Jan 21, 2025
51 checks passed
@mkllnk mkllnk deleted the police-migrations branch January 21, 2025 00:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
technical changes only These pull requests do not contain user facing changes and are grouped in release notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants