Differential D5298

deposit.cli: Fix cli parsing issue when xml error is returned by server
ClosedPublic
Actions

Authored by ardumont on Mar 22 2021, 4:00 PM.

Details

Reviewers

anlambert

Group Reviewers

Reviewers

Commits

rDDEP026ca4ead098: deposit.cli: Fix cli parsing issue when xml error is returned by server

Summary

Prior to this commit, any error returned by the deposit server would end up a
cascading KeyError [1]

This commit fixes it.

I'm not sure whether we want to keep the existing behavior. I don't think that old code
is possible to reach.

[1] P980

Test Plan

one test added

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.Mar 22 2021, 4:00 PM

Build is green

Patch application report for D5298 (id=18981)

Rebasing onto 1c20fcc561...

Current branch diff-target is up to date.

Changes applied before test

commit f850eff5d8424a975c28bce6996a7ae65a025629
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Mar 22 15:57:21 2021 +0100

    deposit.cli: Fix cli parsing issue when xml error is returned by server
    
    Prior to this commit, any error returned by the deposit server would end up a cascading
    KeyError.
    
    This commit fixes it.

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

Harbormaster completed remote builds in B20059: Diff 18981.Mar 22 2021, 4:02 PM

ardumont requested review of this revision.Mar 22 2021, 4:02 PM

ardumont added a subscriber: vlorentz.Mar 22 2021, 4:03 PM

ardumont added inline comments.

swh/deposit/client.py
318	Like i said in the main diff description, i think this should go away as it's no longer reachable. @vlorentz thoughts?
swh/deposit/tests/data/atom/error-cli.xml
9	This mimicks exactly what the deposit server sends the deposit client cli when an error occurs.

ardumont added inline comments.Mar 22 2021, 4:06 PM

swh/deposit/client.py
313	Why does that file does not appear covered?

ardumont edited the summary of this revision. (Show Details)Mar 22 2021, 4:08 PM

Looks good to me.

swh/deposit/client.py
313	Phabricator bug ? The coverage report on Jenkins is well available: https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/584/coverage/cobertura__coverage_xml/project/_/client_py/

This revision is now accepted and ready to land.Mar 22 2021, 4:17 PM

ardumont added inline comments.Mar 22 2021, 4:58 PM

swh/deposit/client.py
313	yes, looks like it [1] Thanks for the link to the actual coverage in jenkins (til ;) [1] It's not the first time something like this happened. I've seen in other diffs apparent missing coverage while it is covered ¯\_(ツ)_/¯

I have no idea what this is about. Does the server send two different error formats now? Is it a bug? If not, why?

swh/deposit/client.py
313	what does this comment mean?

ardumont added inline comments.Mar 22 2021, 5:27 PM

swh/deposit/client.py
313	lol, it's a comment more meant for the diff than for the code. I'll remove. It means, this behaves as before (see on your left). That code is also on its way to be removed as i convinced myself it's unreachable.

I have no idea what this is about.

Have you read the diff description and the associated paste?
I have put effort into describing what this is about so i'm a bit
at a loss on what i can do better here.

Does the server send two different error formats now?

No, it's the same as before... but... see below.

Is it a bug?

Yes, thus the verb "fix" in the commit message and the diff description.

The deposit cli currently fails to parse the error message sent by the
deposit server.

I gather it's bugged since when we introduced the right way to parse xml
with the namespaces and everything. And it only appears when a real
problem is issued by the server.

I noticed it last week when i deployed the integration test in staging and
the new deposit v0.12. Which, strangely, also prevents the report to be
sent to the sysadm irc channel but that's a story for another time ;)

Drop the else statement, unreachable code.

Drop spurious change

Build is green

Patch application report for D5298 (id=18984)

Rebasing onto 1c20fcc561...

Current branch diff-target is up to date.

Changes applied before test

commit 8c575b3038d99d391d4d969907290e927f62f940
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Mar 22 15:57:21 2021 +0100

    deposit.cli: Fix cli parsing issue when xml error is returned by server
    
    Prior to this commit, any error returned by the deposit server would end up a cascading
    KeyError.
    
    This commit fixes it.

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

Harbormaster completed remote builds in B20061: Diff 18984.Mar 22 2021, 5:42 PM

Build is green

Patch application report for D5298 (id=18985)

Rebasing onto 1c20fcc561...

Current branch diff-target is up to date.

Changes applied before test

commit 026ca4ead098bd3ddb8b8440ca0a77f50651debb
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Mar 22 15:57:21 2021 +0100

    deposit.cli: Fix cli parsing issue when xml error is returned by server
    
    Prior to this commit, any error returned by the deposit server would end up a cascading
    KeyError.
    
    This commit fixes it.

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

Harbormaster completed remote builds in B20062: Diff 18985.Mar 22 2021, 5:44 PM

Closed by commit rDDEP026ca4ead098: deposit.cli: Fix cli parsing issue when xml error is returned by server (authored by ardumont). · Explain WhyMar 23 2021, 11:28 AM

This revision was automatically updated to reflect the committed changes.

ardumont added a commit: rDDEP026ca4ead098: deposit.cli: Fix cli parsing issue when xml error is returned by server.