Page MenuHomeSoftware Heritage

[WIP] Handle alias targets in a Branch
Needs ReviewPublic

Authored by jayeshv on Jan 5 2023, 8:36 PM.

Details

Reviewers
anlambert
Group Reviewers
Reviewers
Summary

Resolve alias targets recursively and return the final target
along with the resolveChain list.
Maximum recursion depth is kept to 5. Return None in case of
a missing or invalid chain.

Diff Detail

Repository
rDGQL GraphQL API
Branch
alias-resolve
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 33430
Build 52402: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 52401: arc lint + arc unit

Event Timeline

Build is green

Patch application report for D9002 (id=32460)

Rebasing onto f2fae75b9c...

Current branch diff-target is up to date.
Changes applied before test
commit 9c46493b5dd6642de0bc23a020e4edd2d4178f4d
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Thu Jan 5 17:16:50 2023 +0100

    Handle alias targets in a Branch
    
    Resolve alias targets recursively and return the final target
    along with the resolveChain list.
    Maximum recursion depth is kept to 5. Return None in case of
    a missing or invalid chain.

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

Build is green

Patch application report for D9002 (id=32461)

Rebasing onto f2fae75b9c...

Current branch diff-target is up to date.
Changes applied before test
commit 70dc6cd402c0a4549fd6b46e6a8c2e20796b9d31
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Thu Jan 5 17:16:50 2023 +0100

    Handle alias targets in a Branch
    
    Resolve alias targets recursively and return the final target
    along with the resolveChain list.
    Maximum recursion depth is kept to 5. Return None in case of
    a missing or invalid chain.

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

extra check to find circular reference

Build is green

Patch application report for D9002 (id=32465)

Rebasing onto f2fae75b9c...

Current branch diff-target is up to date.
Changes applied before test
commit 83bd2126b6722a1a3bf809eaad4183e4fd361c16
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Thu Jan 5 17:16:50 2023 +0100

    Handle alias targets in a Branch
    
    Resolve alias targets recursively and return the final target
    along with the resolveChain list.
    Maximum recursion depth is kept to 5. Return None in case of
    a missing or invalid chain.

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

Build is green

Patch application report for D9002 (id=32466)

Rebasing onto f2fae75b9c...

Current branch diff-target is up to date.
Changes applied before test
commit 4628b57e1e5651f7576451fd12d3333a1a221b0f
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Thu Jan 5 17:16:50 2023 +0100

    Handle alias targets in a Branch
    
    Resolve alias targets recursively and return the final target
    along with the resolveChain list.
    Maximum recursion depth is kept to 5. Return None in case of
    a missing or invalid chain.

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

vlorentz added inline comments.
swh/graphql/resolvers/snapshot_branch.py
31–33
35–41

Why the annotations here? They should be inferred by mypy based on the functions' return types

swh/graphql/schema/schema.graphql
527

not sure what ??? should be though.

swh/graphql/resolvers/snapshot_branch.py
35–41

The if condition beneath is a bit too complex with many checks. The first check (if not alias_branches) is only because the return type here could be None.
Added this to make it clear here, but I think is not really needed. I will remove. Anyway, I will change this function to a recursive one :)

anlambert added a subscriber: anlambert.

See my inline comment, better improving code we already have to reuse it than reimplementing.

swh/graphql/resolvers/snapshot_branch.py
25–56

We already have a tested snapshot_resolve_alias function in swh-storage to recursively resolve a branch alias. It does not return the chain of aliases though but only the resolved alias.

I think it would be better to reuse that function instead of duplicating code.
Nevertheless, it should be modified to also return the chain of resolved alias to meet your requirements.
Currently only swh-webuses that function so the impact of this change should be minimal.

This revision now requires changes to proceed.Jan 6 2023, 12:36 PM

Build is green

Patch application report for D9002 (id=32467)

Rebasing onto f2fae75b9c...

Current branch diff-target is up to date.
Changes applied before test
commit 159c11470a1ca0ddab5ad1a4f4b681d18073cca9
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Thu Jan 5 17:16:50 2023 +0100

    Handle alias targets in a Branch
    
    Resolve alias targets recursively and return the final target
    along with the resolveChain list.
    Maximum recursion depth is kept to 5. Return None in case of
    a missing or invalid chain.

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

swh/graphql/resolvers/snapshot_branch.py
25–56

Thanks, I missed that. will modify that function instead.

recursive logic for branch target resolve; to be moved to storage

jayeshv retitled this revision from Handle alias targets in a Branch to [WIP] Handle alias targets in a Branch.Jan 6 2023, 2:11 PM

Build is green

Patch application report for D9002 (id=32469)

Rebasing onto f2fae75b9c...

Current branch diff-target is up to date.
Changes applied before test
commit 02786e52bdc21890528520d5ec50179bb10bcddc
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Thu Jan 5 17:16:50 2023 +0100

    Handle alias targets in a Branch
    
    Resolve alias targets recursively and return the final target
    along with the resolveChain list.
    Maximum recursion depth is kept to 5. Return None in case of
    a missing or invalid chain.

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