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

Make data migrations idempotent #2037

Closed
wants to merge 1 commit into from

Conversation

mattwr18
Copy link
Contributor

No description provided.

@mattwr18 mattwr18 requested a review from soey September 19, 2024 08:29
Copy link
Contributor

@soey soey left a comment

Choose a reason for hiding this comment

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

Ouch ..yes .. data migrations and code .. they are still painful. I would be even more pessimistic and add safeguards also for the cases if we have multiple organizations

@@ -3,7 +3,7 @@
class AddOrganizationIdToRequests < ActiveRecord::Migration[6.1]
def up
ActiveRecord::Base.transaction do
organization = Organization.singleton
organization = Organization.first
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens later on instances when there are multiple organizations? I would even add sth like return if Organization.count != 1 to ensure that it is only run with exactly one organization

@@ -16,7 +16,7 @@ def up

def down
ActiveRecord::Base.transaction do
organization = Organization.singleton
organization = Organization.first
return unless organization
Copy link
Contributor

Choose a reason for hiding this comment

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

see above

@@ -3,7 +3,7 @@
class AddOrganizationIdToActivityNotifications < ActiveRecord::Migration[6.1]
def up
ActiveRecord::Base.transaction do
organization = Organization.singleton
organization = Organization.first
return unless organization
Copy link
Contributor

Choose a reason for hiding this comment

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

see above

@@ -16,7 +16,7 @@ def up

def down
ActiveRecord::Base.transaction do
organization = Organization.singleton
organization = Organization.first
return unless organization
Copy link
Contributor

Choose a reason for hiding this comment

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

see above

@@ -3,7 +3,7 @@
class AddOrganizationIdToTaggings < ActiveRecord::Migration[6.1]
def up
ActiveRecord::Base.transaction do
organization = Organization.singleton
organization = Organization.first
return unless organization
Copy link
Contributor

Choose a reason for hiding this comment

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

see above

@@ -16,7 +16,7 @@ def up

def down
ActiveRecord::Base.transaction do
organization = Organization.singleton
organization = Organization.first
return unless organization
Copy link
Contributor

Choose a reason for hiding this comment

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

see above

@mattwr18
Copy link
Contributor Author

Ouch ..yes .. data migrations and code .. they are still painful. I would be even more pessimistic and add safeguards also for the cases if we have multiple organizations

this is a good point and makes me want to remove this data migrate gem and go back to the temp rake tasks/ansible plays, which can be run once on whichever instances they need to be run and aren't run automatically on deploy.

@mattwr18
Copy link
Contributor Author

Superseded by #2042

@mattwr18 mattwr18 closed this Sep 26, 2024
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