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(robot-server): update data_files_table with uploaded source #16813

Merged

Conversation

TamarZanzouri
Copy link
Contributor

Overview

bug fix for https://opentrons.atlassian.net/browse/RABR-665.
update db row instead of inserting

Test Plan and Hands on Testing

upgrade to the next alpha on ABR9 (10.14.19.41) and ABR12 (10.14.19.236) and make sure migration is complete.

Changelog

update sql instead of insert.

Review requests

@TamarZanzouri TamarZanzouri requested a review from a team as a code owner November 14, 2024 15:40
Copy link
Contributor

@SyntaxColoring SyntaxColoring 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 the quick investigation. This makes sense.

I think we must test this before merging. Ideally with an automated snapshot-based test, but manually works too for the sake of cutting the alpha. The old code was (apparently?) not covered by any test at all, so we don’t want to repeat that.

Copy link
Contributor

@vegano1 vegano1 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 fixing this so quickly.

@TamarZanzouri
Copy link
Contributor Author

Thanks for the quick investigation. This makes sense.

I think we must test this before merging. Ideally with an automated snapshot-based test, but manually works too for the sake of cutting the alpha. The old code was (apparently?) not covered by any test at all, so we don’t want to repeat that.

totally agree! I will test this manually for now but we need a test for this

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Agree with @SyntaxColoring re: testing, and thank you for fixing!

@vegano1
Copy link
Contributor

vegano1 commented Nov 14, 2024

Agree on the testing, ill add a test to cover this case.

@SyntaxColoring
Copy link
Contributor

Tested on my laptop with persistence directories extracted from a couple of ABR robots. (I've attached the .zips to the ticket.) The new migration completes successfully on both. I haven't checked if the new data is semantically correct and that you can use the files in runs, etc.

@TamarZanzouri TamarZanzouri merged commit 6d62bec into chore_release-8.2.0 Nov 14, 2024
13 checks passed
@TamarZanzouri TamarZanzouri deleted the RABR-665-pvt-1-abr-11-migration-fix branch November 14, 2024 18:26
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.

4 participants