Project

Profile

Help

Task #7908

Make sure all exceptions live in pulpcore.plugin.exceptions

Added by ggainey 12 months ago. Updated 12 months ago.

Status:
CLOSED - CURRENTRELEASE
Priority:
Low
Assignee:
-
Category:
-
Sprint/Milestone:
Start date:
Due date:
% Done:

100%

Estimated time:
Platform Release:
Groomed:
No
Sprint Candidate:
No
Tags:
Sprint:
Sprint 86
Quarter:

Description

See discussion at https://github.com/pulp/pulpcore/pull/1027#discussion_r524551412

This will require a deprecation cycle, with a given exception living "in two places" for one release. Once this issue is in MODIFIED, open a new issue to complete the move.

Associated revisions

Revision 86fd56f4 View on GitHub
Added by ggainey 12 months ago

Moved UnsupportedDigestValidationError to new home.

It now lives in pulpcore.plugin.exceptions.

closes #7908

History

#1 Updated by daviddavis 12 months ago

  • Sprint/Milestone set to 3.9.0

#2 Updated by fao89 12 months ago

  • Tracker changed from Issue to Task
  • % Done set to 0
  • Severity deleted (2. Medium)
  • Triaged deleted (No)
  • Sprint set to Sprint 86

#3 Updated by ggainey 12 months ago

We'll want to document this in the plugin-writer's guide and at CHANGES/plugin_api/7908.feature, so that plugin-authors will know to look for exceptions available at pulpcore.plugin.exceptions

#4 Updated by ggainey 12 months ago

The only two Exception-definitions I have found outside of pulpcore.exceptions:

  • pulpcore/content/handler.py:class ArtifactNotFound(Exception):
  • pulpcore/app/models/content.py:class UnsupportedDigestValidationError(Exception):

pulpcore.content.handler.ArtifactNotFound appears to not be used anywhere.

There exists a pulp_container.app.registry.ArtifactNotFound(Exception), whichis unrelated (other than the name)

Proposal:

  • Delete pulpcore.content.handler.ArtifactNotFound as dead code
  • Create a pulpcore.exceptions.validation.UnsupportedDigestValidationError
  • Make that UnsupportedDigestValidationError available to pulpcore.plugin.exceptions
  • Update all of pulpcore to find UnsupportedDigestValidationError from its new location
  • Mark pulpcore.app.models.UnsupportedDigestValidationError as deprecated via announcement in CHANGES

In 3.10, remove pulpcore.app.models.UnsupportedDigestValidationError

#5 Updated by bmbouter 12 months ago

Since pulpcore.app.models.UnsupportedDigestValidationError is not part of the pulpcore.plugin package, we don't need to put it through the deprecation cycle. Except for this 1 change I think the plan you outline is what should be done. I don't think this change will require any deprecation at all actually.

#6 Updated by daviddavis 12 months ago

One thing that is unclear to me: is pulpcore.plugin.models.UnsupportedDigestValidationError remaining as is? I don't see anything in the plan about that and it would be kind of redundant with the step to "Make that UnsupportedDigestValidationError available to pulpcore.plugin.exceptions".

#7 Updated by ggainey 12 months ago

daviddavis wrote:

One thing that is unclear to me: is pulpcore.plugin.models.UnsupportedDigestValidationError remaining as is? I don't see anything in the plan about that and it would be kind of redundant with the step to "Make that UnsupportedDigestValidationError available to pulpcore.plugin.exceptions".

OK, so there are two needs here. One is to keep all the exceptions in one place, and the other is to make them accessible 'outside' of pulpcore itself. To satisfy these, UnsupportedDigestValidationError needs to move to pulpcore.plugin.exceptions.

The only problem with "just moving it", is if there exists non-pulpcore code that depends on finding it at pulpcore.plugin.models, they'll break. To be correct, we should :

  • make it available at pulpcore.plugins.exceptions,
  • leave it available at pulpcore.plugins.models, imported from its new location
  • announce the pulpcore.plugin.models one is deprecated, and then
  • remove that one in 3.10.

Whew. Does that match everyone's expectations?

#8 Updated by daviddavis 12 months ago

Whew. Does that match everyone's expectations?

Indeed it does.

#9 Updated by pulpbot 12 months ago

  • Status changed from NEW to POST

#10 Updated by ggainey 12 months ago

  • Status changed from POST to MODIFIED
  • % Done changed from 0 to 100

#11 Updated by pulpbot 12 months ago

  • Status changed from MODIFIED to CLOSED - CURRENTRELEASE

Please register to edit this issue

Also available in: Atom PDF