Project

Profile

Help

Refactor #1142

MetadataFileContext in pulp and pulp_rpm

Added by jluza over 6 years ago. Updated over 2 years ago.

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

100%

Estimated time:
Platform Release:
Groomed:
No
Sprint Candidate:
No
Tags:
Pulp 2
Sprint:
Quarter:

Description

I discovered, there are two classes MetadataFileContext. One in pulp_rpm/plugins/pulp_rpm/plugins/distributors/yum/metadata/metadata.py and the other in pulp/server/pulp/plugins/util/metadata_writer.py. For updateinfo, comps and repomd is used one from pulp_rpm and for primary, filelists and other(fast-forward content) is used one from pulp. But point is, those two classes are almost the same. There're only minor differences (One has _write_root_tag_open in initialize, another hasn't. One has exclude for REPOMD_FILE_NAME, another hasn't. One has empty write_xml_header, another hasn't). From my point of view, it looks like one of the classes should be obsolete. But actually both are in use.
Current situation is quite confusing and makes pulp hard to debug. I suggest, one of classes should inherits from the other.

Associated revisions

Revision eb322da7 View on GitHub
Added by semyers over 5 years ago

Refector to use MetadataFileContext from platform

fixes #1142 https://pulp.plan.io/issues/1142

History

#1 Updated by jortel@redhat.com over 6 years ago

  • Tracker changed from Issue to Refactor
  • Priority changed from Normal to Low
  • Groomed set to No
  • Sprint Candidate set to No

Investigate to see if the class in pulp_rpm can be removed and the plugin refactored to use the one in platform. If this is the case, change the project on this issue to PULP_RPM.

#2 Updated by bmbouter almost 6 years ago

  • Parent task set to #1683

#3 Updated by mhrivnak almost 6 years ago

  • Sprint/Milestone set to 19

#4 Updated by semyers almost 6 years ago

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

/me puts on a deerstalker cap and begins to investigate

#5 Updated by mhrivnak over 5 years ago

  • Sprint/Milestone changed from 19 to 20

#6 Updated by semyers over 5 years ago

Since both classes are actually in-use, and also both have subclasses, I'm bringing the newer behavior from pulp_rpm back into platform and making the pulp_rpm version be a subclass of it that contains the tiny slice of yum-specific behavior that has been added to pulp_rpm.

It looks trivial to maintain backward-compatibility, so existing calls to this class and its subclasses in pulp_rpm won't need to be modified to adapt to the changes I'm making.

In addition to repetition in these two classes, many tests have been duplicated. I'm also going through an auditing those to similarly DRY things up.

#7 Updated by semyers over 5 years ago

Ah, and I forgot to mention that this change will require two PRs, one for platform and one for RPM.

#8 Updated by semyers over 5 years ago

semyers wrote:

In addition to repetition in these two classes, many tests have been duplicated. I'm also going through an auditing those to similarly DRY things up.

Naturally, this is the part that's taking the most time :(

#9 Updated by semyers over 5 years ago

  • Status changed from ASSIGNED to POST

Platform PR:
https://github.com/pulp/pulp/pull/2565

RPM branch that can be a PR once the platform PR is merged:
https://github.com/pulp/pulp_rpm/compare/master...seandst:rm1142

#10 Updated by semyers over 5 years ago

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

#11 Updated by semyers over 5 years ago

  • Status changed from MODIFIED to ASSIGNED

My changes broke master, so I'll need to tone this down a little bit. It looked like changing some method 'pass'es to raising a notimplementederror was particularly rude.

#12 Updated by semyers over 5 years ago

  • Status changed from ASSIGNED to NEW
  • Assignee deleted (semyers)

Sorry to drop this, but I don't think I'll be able to get to it in the immediate future.

These changes passed unit tests but broke pulp-smash, so some more work is needed to consolidate the common behavior into platform.

#13 Updated by bmbouter over 5 years ago

  • Sprint/Milestone deleted (20)

Unsetting the sprint since it is not currently being worked on.

#14 Updated by bmbouter over 4 years ago

  • Tags RCM added

#15 Updated by bmbouter almost 3 years ago

  • Status changed from NEW to CLOSED - WONTFIX

#16 Updated by bmbouter almost 3 years ago

Pulp 2 is approaching maintenance mode, and this Pulp 2 ticket is not being actively worked on. As such, it is being closed as WONTFIX. Pulp 2 is still accepting contributions though, so if you want to contribute a fix for this ticket, please reopen or comment on it. If you don't have permissions to reopen this ticket, or you want to discuss an issue, please reach out via the developer mailing list.

#17 Updated by bmbouter almost 3 years ago

  • Tags Pulp 2 added

#18 Updated by bmbouter over 2 years ago

  • Tags deleted (RCM)

Also available in: Atom PDF