The Puppet Labs Issue Tracker has Moved: https://tickets.puppetlabs.com

Refactor #16008

The RAL and Parser resource and Type classes should be refactored and merged

Added by Luke Kanies over 2 years ago. Updated over 2 years ago.

Status:InvestigatingStart date:08/17/2012
Priority:NormalDue date:
Assignee:eric sorenson% Done:

0%

Category:RAL
Target version:-
Affected Puppet version:2.7.19 Branch:
Keywords:

We've Moved!

Ticket tracking is now hosted in JIRA: https://tickets.puppetlabs.com

This ticket may be automatically exported to the PUP project on JIRA using the button below:


Description

Right now we have two resource type classes (Puppet::Type and Puppet::Resource::Type) and three resource classes (Puppet::Type, Puppet::Resource, and Puppet::Parser::Resource). The parser resource class is a subclass of Puppet::Resource, but it’s still another class. In addition, we have two ways of managing collections of types — Puppet::Resource::TypeCollection in the parser, and Puppet::Type in the RAL.

These classes should be reduced to one Type class and one Resource class, along with one mechanism for managing collections of Types.

The benefits of this simplification are many:

  • Sheer maintainability. The current RAL and parser resource stuff is largely calcified. It’s barely been changed in years, and nearly everyone finds it impossible to understand or maintain.

  • Reduced duplication. There is a large amount of painful duplication or near duplication in the classes – they all define mechanisms for defining parameters, validating them, etc.

  • Cleaner code. The Puppet::Type class is a mess. It is a class management system (for managing subclasses of Puppet::Type), a class of classes (for defining behavior of subclasses), and a class itself (for defining behavior of resources). These three behaviors should be split into three classes, like is done in the parser. The classes of Puppet::Resource, Puppet::Resource::Type, and Puppet::Resource::TypeCollection are dramatically easier to understand and maintain than Puppet::Type.

I have taken multiple cuts at this (e.g., https://github.com/lak/puppet/tree/prototype/2.7.x/ral_refactor and https://github.com/lak/puppet/tree/prototype/master/ral_refactor_after_param_refactor), but it’s a bunch of work, and there are some fundamental problems in the tests that prevented me from getting it done.

My recommended route, as you’ll see in my code, is that we split Puppet::Type into the three component classes (I chose OldResource, OldType, and OldTypeCollection, I think, for the class names), then merge the respective classes.

Note that one of the major benefits of this refactor is that resource types move from being classes, which are global (and thus cannot be environment-specific), to being instances, which means they work fine in different environments. Providers would still be classes and thus cross-environment, but that could be fixed, too.


Related issues

Related to Puppet - Refactor #8236: Transaction resource harness should interact directly wit... Code Insufficient 07/05/2011
Related to Puppet - Bug #12173: Masters cannot reliably distinguish between multiple vers... Accepted 01/25/2012
Related to Puppet - Refactor #8233: Puppet::Type parameter handling should be cleaner In Topic Branch Pending Review 07/05/2011

History

#1 Updated by Luke Kanies over 2 years ago

I marked this as related to #8236, because having the transaction talk directly to the providers would mean that you have far less code to port from the RAL to the new resource classes, and I think it’s the better design in the long run.

#2 Updated by eric sorenson over 2 years ago

  • Status changed from Unreviewed to Investigating

Also available in: Atom PDF