Project

Profile

Help

Issue #6555

closed

Story #6134: [EPIC] Pulp import/export

Investigate/decide whether "set last_export to null explicitly before allowing exporter to be deleted" is a Good Idea

Added by ggainey over 4 years ago. Updated over 4 years ago.

Status:
CLOSED - CURRENTRELEASE
Priority:
Normal
Assignee:
Category:
-
Sprint/Milestone:
Start date:
Due date:
Estimated time:
Severity:
2. Medium
Version:
Platform Release:
OS:
Triaged:
Yes
Groomed:
Yes
Sprint Candidate:
No
Tags:
Sprint:
Sprint 77
Quarter:

Description

From discussion started while writing functional tests :

pulpcore/tests/functional/api/using_plugin/test_pulpexport.py
        if path.exists(exporter.path):
            shutil.rmtree(exporter.path)
        # NOTE: you have to manually undo 'last-export' if you really really REALLY want to
        #  delete an Exporter. This is...probably correct?
 @daviddavis
Hmm. I think deleting the exporter should just cascade delete the exports?

 @ggainey
We protect from deleting the last_export of a PulpExporter (see https://github.com/pulp/pulpcore/blob/master/pulpcore/app/models/exporter.py#L198) - iirc that kept PulpExporter from being deleted if it had ever been exported, unless/until you explicitly set last_export to null first. I will experiment some more tho, a lot happened between when I wrote that comment and now.

 @daviddavis
Ok. I do see value in preventing a user from accidentally deleting an exporter that has exported but unsetting a field to delete an object seems weird.

 @ggainey
You are not wrong. There is probably a better way to do this, will leave this comment open and think about it overnight.

 @daviddavis
Ok sounds good. I'm fine with leaving it this way as long as it's clear to the user that the last_export has to be unset in order to delete.

Decide if this is appropriate. If not, decide what the right thing to do is, and use this issue to get the change checked in.

Actions #1

Updated by rchan over 4 years ago

  • Sprint changed from Sprint 71 to Sprint 72
Actions #2

Updated by rchan over 4 years ago

  • Sprint changed from Sprint 72 to Sprint 73
Actions #3

Updated by rchan over 4 years ago

  • Sprint changed from Sprint 73 to Sprint 74
Actions #4

Updated by rchan over 4 years ago

  • Sprint changed from Sprint 74 to Sprint 75
Actions #5

Updated by rchan over 4 years ago

  • Sprint changed from Sprint 75 to Sprint 76
Actions #6

Updated by rchan over 4 years ago

  • Sprint changed from Sprint 76 to Sprint 77
Actions #7

Updated by ggainey over 4 years ago

  • Status changed from NEW to ASSIGNED
  • Assignee set to ggainey

@daviddavis - having been bitten by this behavior a number of times now, I think I have convinced myself that 'special protection' for last_export is a) not actually all that useful, and b) very, very inconvenient for the main workflow of deleting an Exporter. With the addition of start_versions= and version=, one can recreate a last-export at will. I think I would like to remove this protection. Objections?

Actions #8

Updated by daviddavis over 4 years ago

I agree. +1

Actions #9

Updated by pulpbot over 4 years ago

  • Status changed from ASSIGNED to POST

Added by ggainey over 4 years ago

Revision e00d176a | View on GitHub

Changed PulpExporter.last_export from PROTECTED to SET_NULL.

fixes #6555

Actions #10

Updated by ggainey over 4 years ago

  • Status changed from POST to MODIFIED

Added by ggainey over 4 years ago

Revision e294b823 | View on GitHub

Changed PulpExporter.last_export from PROTECTED to SET_NULL.

fixes #6555

(cherry picked from commit e00d176a90783cc757de025cb5be4f5b810645bc)

Actions #12

Updated by dkliban@redhat.com over 4 years ago

  • Sprint/Milestone set to 3.6.0
Actions #13

Updated by pulpbot over 4 years ago

  • Status changed from MODIFIED to CLOSED - CURRENTRELEASE

Also available in: Atom PDF