Page MenuHomeSoftware Heritage
Differential D4101

Simplify and document actual information returned to the loader
ClosedPublic
Actions

Authored by ardumont on Sep 30 2020, 2:32 PM.

Details

Reviewers
anlambert
vlorentz
Group Reviewers
Reviewers
Maniphest Tasks
T2649: Modify deposit loader to send metadata from a content-deposit in raw format from to ERMDS
Commits
rDDEPe15f23dc0a5e: Simplify and document actual information returned to the loader
Summary

This is a breaking change which needs adaptation in the deposit loader (D4105).

This moved the provider, tool and raw_metadata keys to the top-level.

(origin in the end is still used by the loader when flushing the metadata in the revision...)

The main reason is to cleanup. Took the opportunity since I need a
deployment for D4100 anyways.

Related to T2649

Test Plan

tox

Diff Detail

Repository
rDDEP Push 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.Sep 30 2020, 2:32 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptSep 30 2020, 2:32 PM
ardumont mentioned this in D4100: deposit: Transit raw metadata to the loader to unify with metadata update scenario.Sep 30 2020, 2:33 PM
ardumont edited the summary of this revision. (Show Details)
swh-public-ci added a comment.Sep 30 2020, 2:36 PM

Build is green

Patch application report for D4101 (id=14452)

Could not rebase; Attempt merge onto 4d72d1be52...

Updating 4d72d1be..137fe219
Fast-forward
 swh/deposit/api/private/__init__.py                |  27 +-
 swh/deposit/api/private/deposit_check.py           |  11 +-
 swh/deposit/api/private/deposit_read.py            |  54 +--
 .../api/test_deposit_private_read_metadata.py      | 393 ++++++---------------
 4 files changed, 164 insertions(+), 321 deletions(-)
Changes applied before test
commit 137fe21904eaae84447979b35ab9b8a97f5b4829
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 30 14:27:24 2020 +0200

    Simplify and document actual information returned to the loader
    
    This is a breaking change which needs adaptation to the loader.
    
    This:
    - drops the no longer used origin keys
    - moved the provider, tool and raw_metadata keys to the top-level
    
    The main reason is to cleanup.
    
    Related to T2649

commit bc012e45ed8b5aa35afd7250c1d3c84204a20033
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 30 14:06:21 2020 +0200

    Transit raw metadata to the loader to unify with metadata update scenario
    
    The new update scenario now stores new metadata update to the metadata storage.
    The loader does not, it currently stores the transformed xml (into json dict).
    The loader passes by this deposit_read call to actually retrieve the data.
    
    So prior to adapting the loader, this needs to happen.
    
    This also:
    - adds type to impacted methods along the way.
    - simplifies a bit the current deposit_read tests
    
    Related to T2649

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/166/ for more details.

Harbormaster completed remote builds in B15730: Diff 14452.Sep 30 2020, 2:36 PM
anlambert added a subscriber: anlambert.Sep 30 2020, 2:51 PM
anlambert added inline comments.
swh/deposit/api/private/deposit_read.py
144

(as) should be removed I think

swh/deposit/tests/api/test_deposit_private_read_metadata.py
127–128

Is this indentation change intended ?

ardumont added inline comments.Sep 30 2020, 3:00 PM
swh/deposit/api/private/deposit_read.py
144

right ;)

swh/deposit/tests/api/test_deposit_private_read_metadata.py
127–128

ah no...

it makes it do it as many times as there are atom keys, thx for noticing!

ardumont edited the summary of this revision. (Show Details)Sep 30 2020, 3:06 PM
ardumont updated this revision to Diff 14454.Sep 30 2020, 3:19 PM

Fix indentation (and add back the origin)

swh-public-ci added a comment.Sep 30 2020, 3:22 PM

Build is green

Patch application report for D4101 (id=14454)

Could not rebase; Attempt merge onto 4d72d1be52...

Updating 4d72d1be..f222108f
Fast-forward
 swh/deposit/api/private/__init__.py                |  27 +-
 swh/deposit/api/private/deposit_check.py           |  11 +-
 swh/deposit/api/private/deposit_read.py            |  57 ++--
 .../api/test_deposit_private_read_metadata.py      | 380 +++++++--------------
 4 files changed, 171 insertions(+), 304 deletions(-)
Changes applied before test
commit f222108f2c3ca51e4b5fd0ce42e2fadba5a399e5
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 30 14:27:24 2020 +0200

    Simplify and document actual information returned to the loader
    
    This is a breaking change which needs adaptation to the loader.
    
    This:
    - drops the no longer used origin keys
    - moved the provider, tool and raw_metadata keys to the top-level
    
    The main reason is to cleanup.
    
    Related to T2649

commit 05df4905ced239a205b5e7c8867354309330b31f
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 30 14:06:21 2020 +0200

    Transit raw metadata to the loader to unify with metadata update scenario
    
    The new update scenario now stores new metadata update to the metadata storage.
    The loader does not, it currently stores the transformed xml (into json dict).
    The loader passes by this deposit_read call to actually retrieve the data.
    
    So prior to adapting the loader, this needs to happen.
    
    This also:
    - adds type to impacted methods along the way.
    - simplifies a bit the current deposit_read tests
    
    Related to T2649

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/168/ for more details.

Harbormaster completed remote builds in B15732: Diff 14454.Sep 30 2020, 3:22 PM
ardumont edited the summary of this revision. (Show Details)Sep 30 2020, 3:22 PM
anlambert accepted this revision.Sep 30 2020, 3:22 PM
This revision is now accepted and ready to land.Sep 30 2020, 3:22 PM
ardumont mentioned this in D4105: deposit: Adapt loader to send extrinsic raw metadata to the metadata storage.Sep 30 2020, 5:59 PM
vlorentz requested changes to this revision.Oct 1 2020, 10:51 AM
vlorentz added a subscriber: vlorentz.

No. We're not adding yet another revision format now.

This revision now requires changes to proceed.Oct 1 2020, 10:51 AM
ardumont added a comment.Oct 1 2020, 10:52 AM

No. We're not adding yet another revision format now.

?

That's not another revision format.

vlorentz added a comment.Oct 1 2020, 10:53 AM

The loader reads this and writes it in the revision, doesn't it?

ardumont added a comment.Oct 1 2020, 11:02 AM

The loader reads this and writes it in the revision, doesn't it?

Yes, it reads it but not writes it directly as is.

As D4105 adaptation shows, their tests are still green and iirc, the metadata format is tested there.

vlorentz added a child revision: D4100: deposit: Transit raw metadata to the loader to unify with metadata update scenario.Oct 1 2020, 11:15 AM
vlorentz added a comment.Oct 1 2020, 11:26 AM

ok, I missed that D4105 reformats the metadata.

But this diff isn't simplifying anything, it just unnests dictionaries in what the API returns, and *adds* complexity to the loader that must undo this change.

ardumont added a comment.EditedOct 1 2020, 11:30 AM

ok, I missed that D4105 reformats the metadata.

But this diff isn't simplifying anything, it just unnests dictionaries in what the API returns,

kinda granted.
But can you tell me what the "origin_metadata" key we came up stood for initially?
I don't understand it so i prefer it goes away and since i'm reworking it to introduce the type change from json to raw... (plus now we'll have both...)
in my mind, might as well clarify intent here.

and *adds* complexity to the loader that must undo this change.

Don't forget i asked about removing altogether that part from the revision though...
Anyway, that "complexity" is temporary.
At some point, we'll finally be able to stop writing to the revision those metadata.

vlorentz accepted this revision.Oct 1 2020, 11:34 AM

fair enough

This revision is now accepted and ready to land.Oct 1 2020, 11:34 AM
ardumont updated this revision to Diff 14475.Oct 1 2020, 12:37 PM

Rebase and adapt according to D4100

swh-public-ci added a comment.Oct 1 2020, 12:40 PM

Build is green

Patch application report for D4101 (id=14475)

Could not rebase; Attempt merge onto 4d72d1be52...

Updating 4d72d1be..e15f23dc
Fast-forward
 swh/deposit/api/private/__init__.py                |  27 +-
 swh/deposit/api/private/deposit_check.py           |  11 +-
 swh/deposit/api/private/deposit_read.py            |  64 ++--
 .../api/test_deposit_private_read_metadata.py      | 391 +++++++--------------
 4 files changed, 184 insertions(+), 309 deletions(-)
Changes applied before test
commit e15f23dc0a5e09507806459f6c2457fccbf33805
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 30 14:27:24 2020 +0200

    Simplify and document actual information returned to the loader
    
    This is a breaking change which needs adaptation to the loader.
    
    This:
    - drops the no longer used origin keys
    - moved the provider, tool and raw_metadata keys to the top-level
    
    The main reason is to cleanup.
    
    Related to T2649

commit 2ecb65d4f534b2ee74d3bb55caa15ee111f8487b
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 30 14:06:21 2020 +0200

    Transit raw metadata to the loader to unify with metadata update scenario
    
    The new update scenario now stores new metadata update to the metadata storage.
    The loader does not, it currently stores the transformed xml (into json dict).
    The loader passes by this deposit_read call to actually retrieve the data.
    
    So prior to adapting the loader, the information returned by deposit_read need
    to provide the raw metadata as well.
    
    This also:
    - adds type to impacted methods along the way.
    - simplifies a bit the current deposit_read tests
    
    Related to T2649

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/170/ for more details.

Harbormaster completed remote builds in B15751: Diff 14475.Oct 1 2020, 12:40 PM
ardumont edited the summary of this revision. (Show Details)Oct 1 2020, 12:40 PM
Closed by commit rDDEPe15f23dc0a5e: Simplify and document actual information returned to the loader (authored by ardumont). · Explain WhyOct 1 2020, 2:42 PM
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDDEPe15f23dc0a5e: Simplify and document actual information returned to the loader.

Revision Contents
Changeset List

PathSize
swh/
deposit/
api/
private/
45 lines
tests/
api/
202 lines

Diff 14491

View Options

swh/deposit/api/private/deposit_read.py

Loading...
View Options

swh/deposit/tests/api/test_deposit_private_read_metadata.py

Loading...
About · Developer information · Legal