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

fix: migration reapply issue fixed for logs #187

Merged
merged 2 commits into from
Oct 11, 2023
Merged

Conversation

nityanandagohain
Copy link
Member

Fixes #186

@srikanthccv
Copy link
Member

Can you edit the existing migration files without getting into checksum issues on instances where they are already applied? Is this needed, given we will have the migrations issue fixed with Dhawal's work soon?

@nityanandagohain
Copy link
Member Author

So it fixes the cases where if the migration is already applied and we reapply the migration then it won't cause any issue. This is independent of dhawals work.

@srikanthccv
Copy link
Member

My first question is not answered. It's not a good idea to edit the existing migrations. It only makes things worse. https://edgeguides.rubyonrails.org/active_record_migrations.html#changing-existing-migrations, this is a RoR guide (I could quickly find a link) but applies across languages/frameworks. If it is a brand new first release, then there is no issue, but there are production users and OSS users who have run it already.

Coming to the second question, does reapplying alone cause this issue? Or does the reapply after the earlier failed migration cause the issue? If it is the latter, then we can assume Dhawal's work will eliminate the possibility of migration failures, and we don't need to do this. If it is the former, the first question is relevant.

@nityanandagohain
Copy link
Member Author

it's the former i.e reapplying the migration causes the issue. Ex- If things are fine and I delete my schema migrations table and restart my collecotor then it won't start and this PR fixes that.

For this case I tested and it doesn't throw any issue as if the migrations are already appliead then this won't even run. Only when all the migrations are rerun this needs to be present.

Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

Thanks for answering my questions.

@nityanandagohain nityanandagohain merged commit 93fddd1 into main Oct 11, 2023
3 checks passed
@nityanandagohain nityanandagohain deleted the issue_186 branch October 11, 2023 10:36
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.

Logs: fix migration 5
2 participants