-
Notifications
You must be signed in to change notification settings - Fork 183
refactor(robot-server): Avoid features that will be removed in SQLAlchemy 2.0 #16926
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
Merged
Merged
Changes from all commits
Commits
Show all changes
70 commits
Select commit
Hold shift + click to select a range
282c99c
WIP intial schema and orchestrator changes
TamarZanzouri 1bb8d7c
changed schema and imports to 8
TamarZanzouri 77902c6
insert the errors to the DB
TamarZanzouri 15db246
refactor into commands table
TamarZanzouri b88e722
select from commands
TamarZanzouri b6d33b6
linting and command error slice done
TamarZanzouri 85eda17
added tests and fixed bugs
TamarZanzouri 5bc0500
changed logic for current command errors
TamarZanzouri e423f37
removed failed_command_errors from state
TamarZanzouri e29d95f
refactor get command errors
TamarZanzouri c275c35
Merge branch 'edge' into EXEC-655-store-commands-error-list-in-db
TamarZanzouri a950cb8
migration and initialization fixes
TamarZanzouri 55a809a
Update robot-server/robot_server/runs/run_store.py
TamarZanzouri 1ccaba3
Merge branch 'edge' into EXEC-655-store-commands-error-list-in-db
TamarZanzouri 9fa97ba
change get_run_errors to get count
TamarZanzouri f5fe016
Merge branch 'edge' into EXEC-655-store-commands-error-list-in-db
TamarZanzouri 13c5b87
tables tests fixed
TamarZanzouri a88e95d
added test for historical run
TamarZanzouri 6fea08a
Merge branch 'edge' into EXEC-655-store-commands-error-list-in-db
TamarZanzouri 1bea28e
fixed persistance path
TamarZanzouri 8f73bc1
fixed import from schema 7
TamarZanzouri c87dc65
rollback add_column change
TamarZanzouri 5405ef3
append_failed_command_id when setting a failed command
TamarZanzouri f888364
append_failed_command_id when setting a failed command
TamarZanzouri e6aa375
moved test to command state
TamarZanzouri 66856ac
changed index and lint
TamarZanzouri 02e6806
WIP - command status
TamarZanzouri 37a4aff
fixed failing tests
TamarZanzouri 6c6257f
insert and get with command_status
TamarZanzouri 56d12c4
translate to CommandStatusSQLEnum
TamarZanzouri c33745b
nips
TamarZanzouri 635a5af
Update robot-server/tests/runs/test_run_store.py
TamarZanzouri a16636b
nits
TamarZanzouri a873369
batch edit
TamarZanzouri 67b6d53
batch update without binding
TamarZanzouri 4d00c99
test table with new index and migration bulk fix
TamarZanzouri 38cd6ea
conditional update
TamarZanzouri 1160174
map command status to sql command status
TamarZanzouri 18ce80c
map command status to sql command status in migration
TamarZanzouri 964fddc
map command status to sql command status in migration
TamarZanzouri 5ad4d0d
add missing indexes
TamarZanzouri de02268
lint
TamarZanzouri 085d4bb
change conversion into enum
TamarZanzouri 378eec1
Merge branch 'edge' into EXEC-655-store-commands-error-list-in-db
TamarZanzouri ec64c12
fixing merge conflicts - WIP
TamarZanzouri b285c99
WIP - fix test_tables failing
TamarZanzouri 3a29bfd
update test table
TamarZanzouri dca8269
Revert "update test table"
TamarZanzouri 4d91d6f
Fix schema_8.py after merge.
SyntaxColoring fd78ad3
Fix up test_tables.py after merge.
SyntaxColoring b59601e
Remove attempts to fix prior migrations, for now.
SyntaxColoring 6496d3c
Make command_status column nullable to match what the migration does.
SyntaxColoring 11aa280
Remove enum constraint, for now.
SyntaxColoring 5af324e
Merge branch 'edge' into EXEC-655-store-commands-error-list-in-db
SyntaxColoring cf88696
Treat SQLAlchemy deprecation warnings as errors.
SyntaxColoring afbe61e
Deduplicate add_column() and avoid deprecated features.
SyntaxColoring f6b61b7
Deduplicate index declaration and avoid executing raw string.
SyntaxColoring 6669e61
Update robot-server/robot_server/persistence/_migrations/v7_to_v8.py
TamarZanzouri ce3fbb3
Update robot-server/robot_server/runs/run_store.py
TamarZanzouri e0aacbd
select subquery and minor fixes
TamarZanzouri aa720f8
We love to lint.
SyntaxColoring b10e8a2
fixed join logic and added logic to the tests
TamarZanzouri 13fd3a8
fixed logic of cursor
TamarZanzouri 55d190e
remove join
TamarZanzouri 68cfdf7
Update robot-server/robot_server/runs/run_store.py
TamarZanzouri 70733de
Merge branch 'edge' into EXEC-655-store-commands-error-list-in-db
TamarZanzouri 86510ec
import from schema_7
TamarZanzouri 73b6ac7
Todo comment for getting SQLAlchemy to compile the text for us.
SyntaxColoring 36186b9
Merge commit '86510ec0383796e8ca9f205b77dbb689aeb3d4ae' into forwards…
SyntaxColoring 5148daf
Merge branch 'edge' into forwards_compatible_sqlalchemy
SyntaxColoring File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol, this is what Alembic is for.
But I wonder if there's a way to use Alembic as a fancy SQL generator for diffs without buying into the full Alembic ecosystem.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is what we thought, but I looked into it when working on this, and I was underwhelmed. Alembic does implement this dance, and that is appealing. But:
First, like you said, there's a full Alembic ecosystem that we'd have to contend with. In particular, we would probably want to run Alembic "inside" our existing migration system. (It can't completely replace our own migration system, because we need to account for migrating regular files, and Alembic can only help with the schema inside the .db file.) Embedding it like that...seems messy? Like, one hypothetical way for it to work would be if Alembic gave us a standalone util function like
alembic.add_all_column_constraints(command_table.c.command_status)
, and we'd call that from within our existing system, but it doesn't seem like Alembic works like that.Second, as far as I can tell, Alembic leaves us on our own for the data part of these migrations, e.g. populating the new non-nullable column. They recommend some general patterns in sqlalchemy/alembic#972 (reply in thread) and https://alembic.sqlalchemy.org/en/latest/cookbook.html#data-migrations-general-techniques. Those patterns strike me as their own dances that are only marginally better. Especially if we need to rearchitect to fit into the Alembic ecosystem just for the privilege of using them.
But I could definitely have big misconceptions about all of this. I've never actually used Alembic for real. If you want to make a sketch or proof of concept to show what it would look like, I'd definitely love something better than what we have now.
Yeah. I'm not sure if this what you're getting at, but there is https://alembic.sqlalchemy.org/en/latest/autogenerate.html, and we might be able to combine that with https://alembic.sqlalchemy.org/en/latest/offline.html. So one option is to run Alembic once on our laptops to autogenerate the
ALTER TABLE
dance, and then manually integrate that with our data migrations. Is that what you have in mind?