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

Remove obsolete check_file_attributes method #647

Merged
merged 1 commit into from
Oct 6, 2024

Conversation

chvp
Copy link
Member

@chvp chvp commented Oct 5, 2024

This was only used in a migration, and new deployments will not actually need it.

This was only used in a migration, and new deployments will not actually need it.
@chvp chvp added the chore Repository or build maintenance label Oct 5, 2024
@chvp chvp requested a review from robbevp October 5, 2024 11:08
Copy link
Member

@robbevp robbevp left a comment

Choose a reason for hiding this comment

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

Not sure I agree with deleting this, since the particular migration still references this function.

Three options:

  • We leave this here
  • We place the content of the method inside the migration
  • We remove old migrations (since new setups should use db:schema:load and we don't support people who haven't updated in 4 years).

I think I prefer option 2 in this case. I know some people who are in favor of the last option but I'm not fully convinced

@chvp
Copy link
Member Author

chvp commented Oct 5, 2024

AFAIK, it is commonly understood that running old migrations will not work in most rails apps. I definitely don't want to go for option 1. Leaving obsolete code in the codebase is just confusing and unnecessary.

I would rather remove the old migrations TBH.

@robbevp
Copy link
Member

robbevp commented Oct 5, 2024

Sure. I just had a look at the migration in question, and it still uses delayed job which wouldn't work anyway. How about we delete all migrations that are more than 2 years old?
We do need an exception for migrations from rails and good job, as their generators could re-create a migration that we already deleted (but with a different timestamp)

@chvp
Copy link
Member Author

chvp commented Oct 5, 2024

Sounds good. Let's do that in a different PR though, maybe?

@robbevp robbevp self-requested a review October 6, 2024 07:09
@robbevp robbevp merged commit 89cf023 into main Oct 6, 2024
4 checks passed
@robbevp robbevp deleted the chore/remove-obsolete-check-file-attributes branch October 6, 2024 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Repository or build maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants