Details

Reviewers

Group Reviewers

Commits

rDSTOC0606791bc865: Stop supporting origin ids in API (except in origin_get_range).
rDSTO0606791bc865: Stop supporting origin ids in API (except in origin_get_range).

Diff Detail

Repository

rDSTO Storage manager

Branch

kill-origin-ids

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 8530
Build 12392: tox-on-jenkins	Jenkins
Build 12391: arc lint + arc unit

Event Timeline

vlorentz created this revision.Oct 17 2019, 7:04 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptOct 17 2019, 7:04 PM

rebase

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

Harbormaster completed remote builds in B8529: Diff 7330.Oct 17 2019, 7:08 PM

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

Harbormaster completed remote builds in B8530: Diff 7331.Oct 17 2019, 7:11 PM

vlorentz mentioned this in D2089: [WIP]Remove origin ids..Oct 17 2019, 10:07 PM

Thanks.

That looks promising.

Just have a couple of docstring fix and interrogation.

swh/storage/in_memory.py
871	`origin (str):`
1494	why don't use rename origin_id here?
swh/storage/storage.py
1102	origin (str): the origin's url
1288	do you still need to do that now? I mean, do we still need to pass the full origin to the journal (including our origin id)?
1495	wow, it's been a while since i looked to the `origin-get` implementation... too many returns/else within the block instructions... It's kind of a mess to read... i'd either go with returns all around and remove the redundant `else` or remove the returns and use a single result variable which i'd return at the bottom... That has nothing to do with the diff though... but since we are touching it... In any case, last return statement, is `res['url'] is None` possible at all here now? I guess yes because we don't know what the url inputted can be?

This revision now requires changes to proceed.Oct 18 2019, 2:35 AM

Just have a couple of docstring fix and interrogation.

Ah and you guessed it, i miss some migration scripts with the *sql functions impacted ;)

vlorentz marked 3 inline comments as done.Oct 18 2019, 11:25 AM

vlorentz added inline comments.

swh/storage/storage.py
1288	That doesn't include the id, but it includes the type, which is currently needed by origin_add
1495	That has nothing to do with the diff though... but since we are touching it... Let's not make this diff even bigger. In any case, last return statement, is res['url'] is None possible at all here now? Yes, if the origin is not already in storage, because missing rows are returned by pg as `(null, null)` because it's a left join instead of inner join.

update docstrings as requested
add migration script

Build has FAILED

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

Harbormaster failed remote builds in B8536: Diff 7338!Oct 18 2019, 11:34 AM

fix argument names in API client.

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

Harbormaster completed remote builds in B8537: Diff 7339.Oct 18 2019, 11:47 AM

ardumont added inline comments.Oct 18 2019, 3:09 PM

swh/storage/storage.py
1288	right!

sounds good.

If @olasd could also have a look, that'd be neat.

This revision is now accepted and ready to land.Oct 18 2019, 6:55 PM

Rebase on top of the origin['type'] removal

Harbormaster failed remote builds in B8618: Diff 7415!Oct 23 2019, 2:05 PM

Build has FAILED

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

olasd added a parent revision: D2173: Remove origin['type'].Oct 23 2019, 2:06 PM

I've rebased this on top of the origin['type'] removal to see whether it could land.

However, this now needs D2174 on swh.model, as the to_dict recursion introduced in OriginVisit breaks when you attr.evlove(OriginVisit, origin=origin_url) (instead of attr.evolve(OriginVisit, origin=Origin(origin_url). This happens in a couple places and is inconsistent with the declared type for OriginVisit.origin.

I guess we should just end up replacing origin with origin_url in OriginVisit objects, as there's not much point carrying a single-key-dict around...

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

Harbormaster completed remote builds in B8618: Diff 7415.Oct 23 2019, 2:38 PM

In D2157#51123, @olasd wrote:

I guess we should just end up replacing origin with origin_url in OriginVisit objects, as there's not much point carrying a single-key-dict around...

Yes, good idea. Should I land this diff as-is, and then we'll handle this renaming next week?

rebase

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

Harbormaster completed remote builds in B8708: Diff 7502.Oct 30 2019, 2:16 PM

vlorentz added a child revision: D2201: Make visit['origin'] a string everywhere (instead of a dict)..Oct 30 2019, 2:37 PM

Closed by commit rDSTO0606791bc865: Stop supporting origin ids in API (except in origin_get_range). (authored by vlorentz). · Explain WhyOct 30 2019, 2:59 PM

This revision was automatically updated to reflect the committed changes.

vlorentz added a commit: rDSTO0606791bc865: Stop supporting origin ids in API (except in origin_get_range)..

vlorentz added a commit: rDSTOC0606791bc865: Stop supporting origin ids in API (except in origin_get_range)..Oct 30 2019, 5:22 PM