Store target's custom meta data when installing #1370
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1370 +/- ##
==========================================
- Coverage 80.27% 80.16% -0.12%
==========================================
Files 178 178
Lines 10567 10582 +15
==========================================
Hits 8483 8483
- Misses 2084 2099 +15
Continue to review full report at Codecov.
|
lbonn
requested review from
lbonn,
mike-sul,
xcheng-here,
Zee314159 and
kbushgit
config/sql/migration/migrate.21.sql
Outdated
|
|
||
| ALTER TABLE installed_versions RENAME TO installed_versions_old; | ||
| CREATE TABLE installed_versions(id INTEGER PRIMARY KEY, ecu_serial TEXT NOT NULL, sha256 TEXT NOT NULL, name TEXT NOT NULL, hashes TEXT NOT NULL, length INTEGER NOT NULL DEFAULT 0, correlation_id TEXT NOT NULL DEFAULT '', is_current INTEGER NOT NULL CHECK (is_current IN (0,1)) DEFAULT 0, is_pending INTEGER NOT NULL CHECK (is_pending IN (0,1)) DEFAULT 0, was_installed INTEGER NOT NULL CHECK (was_installed IN (0,1)) DEFAULT 0, custom_meta TEXT NOT NULL DEFAULT ""); | ||
| INSERT INTO installed_versions(ecu_serial, sha256, name, hashes, length, correlation_id, is_current, is_pending, was_installed) SELECT installed_versions_old.ecu_serial, installed_versions_old.sha256, installed_versions_old.name, installed_versions_old.hashes, installed_versions_old.length, installed_versions_old.correlation_id, installed_versions_old.is_current, installed_versions_old.is_pending, installed_versions_old.was_installed FROM installed_versions_old ORDER BY rowid; |
There was a problem hiding this comment.
You can directly order by id instead of rowid.
Also, it looks like a good use case for ALTER TABLE ADD COLUMN: https://sqlite.org/lang_altertable.html.
There was a problem hiding this comment.
I could have sworn this was failing a migration test when I tried it yesterday. But I just changed to alter column did a clean build and it works. I did a force-push update and hopefully CI will be happy.
As more people take advantage of the custom dictionary value for a Target, we'll need to keep track of this so things like package managers can reason about things correctly. Signed-off-by: Andy Doan <[email protected]>
Signed-off-by: Andy Doan <[email protected]>
There was a problem hiding this comment.
Looks fine to me, thanks!
@lbonn do you mind doing a final check as well before we merge, since you've worked with this code more closely and recently than me?
|
Sure, I've actually already taken a look and it seems fine on a correctness perspective. |
This is my stab at fixing #1292.