Page MenuHomeSoftware Heritage
Differential D1411

Refactor deposit cli and some other improvments
ClosedPublic
Actions

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

Details

Reviewers
douardda
Group Reviewers
Reviewers
Commits
rDDEP2583b6456879: model: Migrate appropriately the DepositRequestType change
rDDEP4ca0bae09d25: settings.production: Allow "allowed" hosts list update
rDDEP3b5582d4b321: admin/user/create: If collection not provided, fallback to username
rDDEP647306d07361: deposit.cli: Rename server part to admin
rDDEP7866eea6afea: config: do not overwrite the DJANGO_SETTINGS_MODULE if already defined
rDDEPae5ff69ca4dd: client: use a global logger
rDDEPbccb60168aa5: cli: kill the unneeded dry-run option
rDDEP130bc968af98: cli/admin/user: add a user existence check
rDDEP968d08c0081b: cli/admin/user: provide more information at creation
rDDEPf5bfbf5c190d: cli: fix service document parsing to retrieve collection
rDDEP4639513ab96e: model: kill DepositRequestType
rDDEP0dfada340f9b: cli: split the cli.py module in a package with submodules
rDDEP0e2c0efb5224: setup: extract server specific requirements in a dedicated 'server' extra
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 Push deposit
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 5411
Build 7333: tox-on-jenkinsJenkins
Build 7332: arc lint + arc unit

Event Timeline

ardumont created this revision.Apr 12 2019, 1:38 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptApr 12 2019, 1:38 PM
swh-public-ci added a comment.Apr 12 2019, 1:42 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DDEP/job/tox/92/
See console output for more information: https://jenkins.softwareheritage.org/job/DDEP/job/tox/92/console

Harbormaster failed remote builds in B5354: Diff 4545!Apr 12 2019, 1:42 PM
ardumont added a subscriber: moranegg.Apr 12 2019, 1:56 PM
ardumont added inline comments.
swh/deposit/cli/deposit.py
133

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

swh-public-ci added a comment.Apr 12 2019, 2:13 PM

Build is green
See https://jenkins.softwareheritage.org/job/DDEP/job/tox/93/ for more details.

Harbormaster completed remote builds in B5354: Diff 4545.Apr 12 2019, 2:13 PM
ardumont added a comment.Apr 12 2019, 3:47 PM

Need to amend some commits back together now.

ardumont mentioned this in T1459: docker container for swh-deposit.Apr 12 2019, 3:52 PM
ardumont added a child revision: D1414: Improve deposit container to actually use loader/checker.Apr 12 2019, 4:02 PM
ardumont updated this revision to Diff 4554.Apr 12 2019, 4:13 PM

Squash commits that belong together

swh-public-ci added a comment.Apr 12 2019, 4:19 PM

Build is green
See https://jenkins.softwareheritage.org/job/DDEP/job/tox/95/ for more details.

Harbormaster completed remote builds in B5405: Diff 4554.Apr 12 2019, 4:19 PM
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
ardumont added a comment.Apr 12 2019, 4:44 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

There you go, should be better.

swh-public-ci added a comment.Apr 12 2019, 7:06 PM

Build is green
See https://jenkins.softwareheritage.org/job/DDEP/job/tox/96/ for more details.

Harbormaster completed remote builds in B5409: Diff 4558.Apr 12 2019, 7:07 PM
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 mentioned this in T1582: deposit: Update debian packages.Apr 12 2019, 11:19 PM
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

swh-public-ci added a comment.Apr 13 2019, 11:17 AM

Build is green
See https://jenkins.softwareheritage.org/job/DDEP/job/tox/97/ for more details.

Harbormaster completed remote builds in B5411: Diff 4561.Apr 13 2019, 11:17 AM
Closed by commit rDDEP0e2c0efb5224: setup: extract server specific requirements in a dedicated 'server' extra (authored by douardda, committed by ardumont). · Explain WhyApr 13 2019, 11:21 AM
This revision was automatically updated to reflect the committed changes.
ardumont added a comment.Apr 17 2019, 9:26 AM
In D1411#30788, @ardumont wrote:

@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.
ardumont mentioned this in T1638: Deposit: error when submitting through cli.Apr 17 2019, 10:02 AM

Revision Contents
Changeset List

PathSize
requirements-server.txt
requirements.txt
3 lines
requirements-swh-server.txt
requirements-swh.txt
1 line
4 lines
2 lines
4 lines
swh/
deposit/
api/
20 lines
private/
4 lines
cli/
37 lines
178 lines
cli/
248 lines
client/
77 lines
15 lines
loader/
4 lines
migrations/
43 lines
25 lines
settings/
2 lines
tests/
api/
6 lines
8 lines
8 lines
34 lines
11 lines
1 line

Diff 4561

View Options

requirements-server.txt

Loading...
View Options

requirements-swh-server.txt

Loading...
View Options

requirements-swh.txt

Loading...
View Options

requirements.txt

Loading...
View Options

setup.py

Loading...
View Options

swh/deposit/api/common.py

Loading...
View Options

swh/deposit/api/private/__init__.py

Loading...
View Options

swh/deposit/cli.py

Loading...
View Options

swh/deposit/cli/__init__.py

Loading...
View Options

swh/deposit/cli/admin.py

Loading...
View Options

swh/deposit/cli/deposit.py

Loading...
View Options

swh/deposit/client/__init__.py

Loading...
View Options

swh/deposit/config.py

Loading...
View Options

swh/deposit/loader/loader.py

Loading...
View Options

swh/deposit/migrations/0015_depositrequest_typemigration.py

Loading...
View Options

swh/deposit/models.py

Loading...
View Options

swh/deposit/settings/production.py

Loading...
View Options

swh/deposit/tests/api/test_deposit_binary.py

Loading...
View Options

swh/deposit/tests/api/test_deposit_delete.py

Loading...
View Options

swh/deposit/tests/api/test_deposit_multipart.py

Loading...
View Options

swh/deposit/tests/api/test_deposit_update.py

Loading...
View Options

swh/deposit/tests/common.py

Loading...
View Options

tox.ini

Loading...
About · Developer information · Legal