Page MenuHomeSoftware Heritage

Refactor deposit cli and some other improvments
ClosedPublic

Authored by ardumont on Apr 12 2019, 1:38 PM.

Details

Summary

This is actually many folds:

  • actual fix on the service document deposit cli which no longer happened.
  • fix cli to separate dependencies perimeter (admin cli, deposit cli)
  • improve the admin cli:
    • to allow more information to be set on the user (provider-url, domain)
    • add a user existence check on the cli (which is used in the docker-env now)
  • improve the user creation cli:
    • add a fallback collection name when not provided to the user name (which matches the current production data)
  • fix some setup configuration issue for the docker-env (allowed-host, etc...)
  • use a global logger in deposit cli
  • migrate the schema model to use simpler schema (Choice instead of table)

There remains (outside the scope of this diff) to:

  • update the documentation about the cli changes
  • update the debian packaging which is most probably broken
  • drop the now unneeded makefile (which was prior to the docker environment use and update the hacking-started on swh documentation)
  • deploy those changes (beware the migration model change!)

Related T1581

Test Plan

tox

Diff Detail

Repository
rDDEP swh-deposit
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ardumont created this revision.Apr 12 2019, 1:38 PM
ardumont added inline comments.
swh/deposit/cli/deposit.py
134 ↗(On Diff #4545)

@moranegg that's the fix we spoke about earlier.

Need to amend some commits back together now.

ardumont updated this revision to Diff 4554.Apr 12 2019, 4:13 PM

Squash commits that belong together

ardumont edited the summary of this revision. (Show Details)Apr 12 2019, 4:22 PM
ardumont edited the summary of this revision. (Show Details)Apr 12 2019, 4:28 PM
douardda accepted this revision.Apr 12 2019, 4:40 PM
douardda added a subscriber: douardda.

lgtm (but I'm obviously biased)

This revision is now accepted and ready to land.Apr 12 2019, 4:40 PM

lgtm (but I'm obviously biased)

yeah, so am i, heh!

ardumont added inline comments.Apr 12 2019, 5:57 PM
swh/deposit/migrations/0015_auto_20190411_1421.py
25 ↗(On Diff #4554)

Excerpt from irc discussion, this needs more work to actually migrate correctly in production.

│16:29:26        +ardumont | the only thing that makes me edgy is the db migration part
│16:29:34        +ardumont | other than that, i'm fine with that diff
│16:29:45        +ardumont | douardda: ^
│16:31:05                * | ardumont documents himself on migration -> https://docs.djangoproject.com/en/2.1/topics/migrations/
│16:39:35        +douardda | ardumont: so let's go
│16:39:53           +olasd | that migration won't work
│16:40:16        +ardumont | there you go!
│16:40:17        +ardumont | ;)
│16:41:13        +ardumont | will it destroy the existing data?
│16:42:33        +ardumont | i see that we can use `django-admin sqlmigrate` (to show the actual executed sql) and possibly `showmigrations` as well
│16:43:18           +olasd | the migration is trying to convert a foreign key (an integer) to a char field with choices
│16:44:03           +olasd | you're going to need to do it in 4 steps: add a new char field, update the data, remove the old field, rename the new field
│16:44:28        +douardda | ardumont olasd: I would have been surprised the migration of django being smart enough to handle this by its own
│16:44:33        +douardda | as I said
│16:44:47        +ardumont | indeed, david said as much yesterday ;)
│16:45:03           +olasd | well I'm just confirming that
│16:45:13        +ardumont | olasd: thx for the heads up
│16:45:17        +ardumont | quite
│16:46:56        +ardumont | well, he is/was dubious django could handle it itself, you are giving us a way forward now!
│16:46:59        +ardumont | so thx both
│16:49:31        +ardumont | > you're going to need to do it in 4 steps: add a new char field, update the data, remove the old field, rename the new field
│16:49:49        +ardumont | olasd: that means 4 modifications in the django model which generate each one a migration file, right?
│16:50:23           +olasd | not really, you should be able to do that in a single migration
│16:50:26           +olasd | +file
ardumont added inline comments.Apr 12 2019, 6:23 PM
swh/deposit/migrations/0015_auto_20190411_1421.py
25 ↗(On Diff #4554)

ok, so we can edit migration files to do thy bidding [1]

You should rarely, if ever, need to edit migration files by hand, but it’s entirely possible to write them manually if you need to. Some of the more complex operations are not autodetectable and are only available via a hand-written migration, so don’t be scared about editing them if you have to.

[1] https://docs.djangoproject.com/en/2.1/topics/migrations/

ardumont added a subscriber: olasd.Apr 12 2019, 6:29 PM
ardumont added inline comments.
swh/deposit/migrations/0015_auto_20190411_1421.py
25 ↗(On Diff #4554)

Better link to reference the quote is [2], last paragraph.

Then [3] for data migration sounds interesting. To actually do the update the data step @olasd mentions \m/

[2] https://docs.djangoproject.com/en/2.1/topics/migrations/#migration-files

[3] https://docs.djangoproject.com/en/2.1/topics/migrations/#data-migrations

ardumont updated this revision to Diff 4558.Apr 12 2019, 7:03 PM
ardumont edited the summary of this revision. (Show Details)
  • model: Migrate appropriately the DepositRequestType change
swh/deposit/migrations/0015_depositrequest_typemigration.py
43 ↗(On Diff #4558)

There you go, should be better.

ardumont added a comment.EditedApr 12 2019, 7:10 PM

@douardda @olasd I have changed the migration to run the 4 steps
mentioned \m/

To do that, i iterated with git (amending) and reused the data files
generated through multiple calls to make db-prepare (wrapper on
swh.deposit.manage makemigrations)...

In the end, I checked the following command to see the sql generated
(in between last version with the old type and the new one):

python3 -m swh.deposit.manage sqlmigrate deposit 0015_depositrequest_typemigration
BEGIN;
--
-- Add field type2 to depositrequest
--
ALTER TABLE "deposit_request" ADD COLUMN "type2" varchar(8) NULL;
--
-- MIGRATION NOW PERFORMS OPERATION THAT CANNOT BE WRITTEN AS SQL:
-- Raw Python operation
--
--
-- Remove field type from depositrequest
--
SET CONSTRAINTS "deposit_request_type_id_e9882b75_fk_deposit_request_type_id" IMMEDIATE; ALTER TABLE "deposit_request" DROP CONSTRAINT "deposit_request_type_id_e9882b75_fk_deposit_request_type_id";
ALTER TABLE "deposit_request" DROP COLUMN "type_id" CASCADE;
--
-- Rename field type2 on depositrequest to type
--
ALTER TABLE "deposit_request" RENAME COLUMN "type2" TO "type";
--
-- Delete model DepositRequestType
--
DROP TABLE "deposit_request_type" CASCADE;
COMMIT;

Expectedly, that does not show the data migration part which happens
through python. But that indeed mentions the migration step ;)

Cheers,

ardumont edited the summary of this revision. (Show Details)Apr 12 2019, 11:23 PM
ardumont updated this revision to Diff 4561.Apr 13 2019, 11:14 AM

Rebase on latest master

This revision was automatically updated to reflect the committed changes.

@douardda @olasd I have changed the migration to run the 4 steps
mentioned \m/
To do that, i iterated with git (amending) and reused the data files
generated through multiple calls to make db-prepare (wrapper on
swh.deposit.manage makemigrations)...
In the end, I checked the following command to see the sql generated
(in between last version with the old type and the new one):

python3 -m swh.deposit.manage sqlmigrate deposit 0015_depositrequest_typemigration
BEGIN;
--
-- Add field type2 to depositrequest
--
ALTER TABLE "deposit_request" ADD COLUMN "type2" varchar(8) NULL;
--
-- MIGRATION NOW PERFORMS OPERATION THAT CANNOT BE WRITTEN AS SQL:
-- Raw Python operation
--
--
-- Remove field type from depositrequest
--
SET CONSTRAINTS "deposit_request_type_id_e9882b75_fk_deposit_request_type_id" IMMEDIATE; ALTER TABLE "deposit_request" DROP CONSTRAINT "deposit_request_type_id_e9882b75_fk_deposit_request_type_id";
ALTER TABLE "deposit_request" DROP COLUMN "type_id" CASCADE;
--
-- Rename field type2 on depositrequest to type
--
ALTER TABLE "deposit_request" RENAME COLUMN "type2" TO "type";
--
-- Delete model DepositRequestType
--
DROP TABLE "deposit_request_type" CASCADE;
COMMIT;

Expectedly, that does not show the data migration part which happens
through python. But that indeed mentions the migration step ;)

I've finally tested this and it worked.

Modus operandi:

  • make a dump of the swh-deposit (currently in the old schema version)
  • restore the dump locally
  • $ workon swh
  • $make db-migrate -> everything is fine
  • and type is now a string as expected.