Page MenuHomeSoftware Heritage

Revert "cli.db: Use attribute current_version instead of undeclared getter"
AbandonedPublic

Authored by ardumont on Thu, Jun 2, 11:42 AM.

Details

Summary

This reverts commit 5cda0ca626010cccc7e7f3f73fbfb1b20702ac40 (D7907) which is not the proper
fix.

The proper fix plan becomes:

  • fix documentation to explicit the get_current_version intention
  • either fix swh-scrubber (and whatever else non-compliants backends) or provide a

default implementation in BaseDb [1] to actually expose that get_current_version (that
can then be overriden if something specific needs to be developed for a new
datastore).

[1] does both by updating swh.core.db

Related to T4284
[1] D7949

Diff Detail

Repository
rDCORE Foundations and core functionalities
Branch
revert-db-migration
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 29709
Build 46428: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 46427: arc lint + arc unit

Event Timeline

Build is green

Patch application report for D7943 (id=28611)

Rebasing onto 4fc5f601b6...

Current branch diff-target is up to date.
Changes applied before test
commit 1ebd38d48e5d5d37c138592639a03ed647fe6360
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Thu Jun 2 11:40:00 2022 +0200

    Revert "cli.db: Use attribute current_version instead of undeclared getter"
    
    This reverts commit 5cda0ca626010cccc7e7f3f73fbfb1b20702ac40. which is not the proper
    fix.
    
    The proper fix plan becomes:
    - fix documentation to explicit the get_current_version intention
    
    - either fix swh-scrubber or provide a default implementation in BaseDb to actually
    expose that get_current_version (that can then be overriden if something specific needs
    to be developed for a new datastore).
    
    Related to T4284

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

Nope, it's fine without this.

11:45 <+ardumont> although for storage, that would have worked
11:45 <+ardumont> well it's working since the implementation is returning that value
11:46 <+ardumont> https://forge.softwareheritage.org/source/swh-storage/browse/master/swh/storage/postgresql/storage.py$208-211
11:46 <+ardumont> which is probably not the implementation you want given what you said before
11:47 <+ardumont> mmm same for scheduler https://forge.softwareheritage.org/source/swh-scheduler/browse/master/swh/scheduler/backend.py$76-78
11:48 <+ardumont> douardda: ^ there is something off between what you said and what's currently implemented
11:48 <+ardumont> The current get_current_version() method implementation are "all" (storage, scheduler so far) returning the current_version attribute
11:48 <+ardumont> so my code actually work (without reverting it)
11:49 <+ardumont> and same for indexer, https://forge.softwareheritage.org/source/swh-indexer/browse/master/swh/indexer/storage/__init__.py$155-157
11:49 <+ardumont> so i stand by my commit

Open it back, it's required in the end. Another fix will happen to propose a default
get_current_version implementation in the BaseDb.

Build is green

Patch application report for D7943 (id=28626)

Rebasing onto e1a1d84eb4...

Current branch diff-target is up to date.
Changes applied before test
commit 1096de5a182968cebbbd98d921814fc332095710
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Thu Jun 2 11:40:00 2022 +0200

    Revert "cli.db: Use attribute current_version instead of undeclared getter"
    
    This reverts commit 5cda0ca626010cccc7e7f3f73fbfb1b20702ac40. which is not the proper
    fix.
    
    The proper fix plan becomes:
    - fix documentation to explicit the get_current_version intention
    
    - either fix swh-scrubber or provide a default implementation in BaseDb to actually
    expose that get_current_version (that can then be overriden if something specific needs
    to be developed for a new datastore).
    
    Related to T4284

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

This revision is now accepted and ready to land.Fri, Jun 3, 10:31 AM

Without this diff, storage and indexer are actually broken since their datastore is missing the current_version attribute.
But it's simpler to not revert that code (abandon this diff) which somehow clarifies a bit what's searched for and the simple
fix is to add their current_version attribute (the one on db.py becomes unneeded).

Diffs incoming

[1] D7943#206331