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

refactor(backend): remove code for deleting db and tables #1864

Merged
merged 4 commits into from
Jul 10, 2024

Conversation

aakrem
Copy link
Collaborator

@aakrem aakrem commented Jul 9, 2024

The code was used when working on migration with main goal is to have a fresh state of tables when we adjust the migration script.

The code was used when working on migration with main goal is to have a fresh state of tables when we adjust the migration script.
Copy link

vercel bot commented Jul 9, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
agenta ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 10, 2024 7:29am

@aakrem aakrem marked this pull request as ready for review July 9, 2024 15:05
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. Backend refactoring labels Jul 9, 2024
@mmabrouk mmabrouk requested a review from aybruhm July 9, 2024 15:10
@aakrem aakrem changed the title refactor(backend): remove code for deleteign db and tables refactor(backend): remove code for deleting db and tables Jul 9, 2024
@aakrem
Copy link
Collaborator Author

aakrem commented Jul 9, 2024

@aybruhm Does it make sense to have a db engine only for tests. or extend it in tests to have the delete db?
I d like to be super safe and remove the delete db from the actual db engine running the app

@aybruhm
Copy link
Member

aybruhm commented Jul 9, 2024

@aybruhm Does it make sense to have a db engine only for tests. or extend it in tests to have the delete db? I d like to be super safe and remove the delete db from the actual db engine running the app

Why are we removing the codes for deleting the db and tables?

Copy link
Member

@aybruhm aybruhm left a comment

Choose a reason for hiding this comment

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

See comment.

@aakrem
Copy link
Collaborator Author

aakrem commented Jul 9, 2024

@aybruhm

  1. If a user migrate agenta to a same db he's using and has a table called users_db apps_db.. (pretty standard names) it will drop them. (edge case but possible)
  2. Again the drop tables are used only for dev purposes so need to have them anymore.
  3. The drop db is used only in tests so why having a method in actual db engine running the app used only for tests.
  4. Why keeping them ?

Copy link
Member

@aybruhm aybruhm left a comment

Choose a reason for hiding this comment

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

Keep this function.

@aybruhm
Copy link
Member

aybruhm commented Jul 9, 2024

@aybruhm

1. If a user migrate agenta to a same db he's using and has a table called users_db apps_db.. (pretty standard names) it will drop them. (edge case but possible)

2. Again the drop tables are used only for dev purposes so need to have them anymore.

3. The drop db is used only in tests so why having a method in actual db engine running the app used only for tests.

4. Why keeping them ?

Got it! Let's not remove the remove_db function yet. I'll take this over to see how it can be done with alembic and update the alembic PR.

@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Jul 10, 2024
Copy link
Member

@aybruhm aybruhm left a comment

Choose a reason for hiding this comment

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

Thank you for PR, @aakrem.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jul 10, 2024
@aakrem aakrem merged commit 12f70a6 into main Jul 10, 2024
9 checks passed
@aakrem aakrem deleted the cleanup/remove-table-deletion-code branch July 10, 2024 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backend lgtm This PR has been approved by a maintainer refactoring size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants