Pulp

Class names need to be unique within any given object model. However, the django models are part of the plugin Data Model and the functional objects are part of the plugin Domain Model. Two completely different object models. The models.FileImporter(Model) IsA Model is completely different than say plugin.FileImporter which is not. They are easily distinguishable by namespace and class hierarchy. What's making the naming hard is the perception that the natural and appropriate name for both is not available. The alternatives, such as FileImporterController, don't seem appropriate because they are contrived just for differentiation.

I'm not dismissing the concern that having 2 classes with same name within a plugin project might be confusing for some developers. My perspective comes from working on other projects with much larger object models than pulp where this was not uncommon or confusing. I don't anticipate convincing anyone and don't want to start a debate. Just wanted to make the point.

jortel@redhat.com wrote:

Class names need to be unique within any given object model. However, the django models are part of the plugin Data Model and the functional objects are part of the plugin Domain Model. Two completely different object models. The models.FileImporter(Model) IsA Model is completely different than say plugin.FileImporter which is not. They are easily distinguishable by namespace and class hierarchy. What's making the naming hard is the perception that the natural and appropriate name for both is not available. The alternatives, such as FileImporterController, don't seem appropriate because they are contrived just for differentiation.

I basically think the same and would be happy for both to have the same class name, easily differentiated by namespaces. That could at least be a fine starting point.

I think using the same name will be confusing. +1 to the controller language. I can't think of anything better. What about these names?

pulpcore.plugin.models.Importer      <---   the importer as the user thinks about it. Optionally subclassed by the plugin writer.
pulpcore.plugin.controllers.ImporterController    <--- the new object the plugin writer would subclass and provide.

pulpcore.plugin.models.Publisher      <---   the publisher as the user think about it. Optionally subclassed by the plugin writer.
pulpcore.plugin.controllers.PublisherController    <--- the new object the plugin writer would subclass and provide.

How will platform discover the subclass of ImporterController and PublisherController? Any plugin can provide multiple importers or distributors and there could be several plugins. How will we know the right one specifically?

Thinking about naming today, and where the convention in django came from of objects that interact with a specific model being named $Modelname$Behavior, I wondered if the genesis of that convention might be ModelForms.

https://docs.djangoproject.com/en/1.11/topics/forms/modelforms/#modelform

The example is clear. myapp.models.Article is an Article. ArticleForm is a form that happens to work with Articles. The name is reasonable even if it turns into myapp.forms.ArticleForm, because the darned thing is a form.

@jortel raised the interesting question: should then the model be named ArticleModel? We concluded no, because the model's main purpose in life is to represent an Article. It is a data structure more than an actor, and it's reasonable to name it after just the thing it represents.

So in short, I talked myself into being more ok with a name like ImporterController.

I wish I could start this comment by offering a better suggestion but, ATM, I don't have one. In any case, Controllers are an anti-pattern in OO and this naming really should be avoided. In most cases, difficulty in selecting a class name suggests that something isn't modeled correctly. Although in this case, the problem is a naming conflict between the pulp (plugin) domain model and the data model. The problem is that both names seem appropriate within their respective models. To resolve this in a way that ignores the different namespaces requires us to change the name of something that already has an appropriate name to something less appropriate.

Looking at the our data model, each class (table) is ,as all objects should be, a noun within the pulp problem domain. For example: User, Repository, Publication, Distribution. Notice nothing (except Importer and Publisher) ends in er. The operations in our django model objects pertain only to persistence which is why these classes are part of the data model. However, our plugin contributed objects deviate from this by ending in er. This naming implies that these objects have operations that are outside the scope of persistence. I believe our naming conflict problem should be resolved by finding a more appropriate name for the Importer and Publisher data model classes. But I'm not sure how. Keep in mind that names that include Config are not appropriate in a data model. Any ideas?

Importers¶

Looking at Importers as they are today, I the fields fall into 2 distinct categories:

• external source config
  • feed_url
  • ssl stuff
  • proxy stuff
  • username/password
• sync settings
  • validate
  • sync_mode
  • download_policy

The external source fields would map nicely to a model called ExternalSource, (or ExternalRepo if you prefer) which would be subclassed by plugins. ex. RPMExternalSource, PythonExternalSource. A user would create an ExternalSource to model each repo that they want to sync. examples: (these would be REST calls, but it is simpler to explain with python)

    RPMExternalSource(name=epel7, feed_url=https://dl.fedoraproject.org/pub/epel/7/x86_64/) 
    PythonExternalSource(name=pypi, feed_url=pypi.python.org)

In my view, an external source would not be owned by a Pulp repo. Multiple repos could have the same external source.

The other half of the Importer (the Synchronizer) is just a behavior that the user doesn't need to know about at all. It is implied by the "type" of the ExternalSource. I'm a lot less concerned about what we call this because it is plugin-writer facing, but not user-facing.

The sync settings need not be modeled at all. A call to sync would look like this:

Python

POST to /v3/repoversions/

post_body = {
    "external_source": "pypi",
    "download_policy": "immediate",
    "sync_mode": "additive",
    "validate": True
    "plugin_settings": {
        "sync_types": ["sdist", "whl"]
        "download_dependencies": True
    }
}

RPM

POST to /v3/repoversions/

post_body = {
    "external_source": "epel7",
    "download_policy": "immediate",
    "sync_mode": "additive",
    "validate": True
    "plugin_settings": {
        "download_dependencies": True
    }
}

If we wanted to, we could store any of the sync options, including plugin_settings, in a `sync_settings = GenericKeyValueField()` on the Repository model. If we want to continue to think of a Repository as a content stream from a source, this would let the users see what a Repository is.

Publishers¶

I am not sure we even need a user-facing Publisher anymore. The Publisher could keep its name, but become a plugin-writer interface. Plugin specific settings can used in the POST body, like for sync. If we wanted to keep a userfacing object for persistent settings, PublishSettings seems right to me.

The proposal of storing plugin (importer) settings on the repository has a cardinality problem. Pulp2 does not support multiple importers but I'm convinced that pulp3 needs to. Also, the example does not provide the platform with information to select a specific plugin (importer). As for the publisher, I believe we need to keep publisher information in the DB as well.

Proposal: Plugins define sync and publish tasks. This make sense because:

1. The plugin API needs to expose the tasking system for complex copy
2. sync and publish are purely plugin activities, and are entirely run in a task

Currently, pulpcore defines an Importer and Publisher master ViewSet, which is the parent of the plugin's detail viewset, FileImporter and FilePublisher. The pulpcore ImporterViewSet and PublisherViewSet define sync and publish endpoints, which deploy a generic sync task that calls sync on the detail model. Instead, the detail ViewSets should deploy their own sync tasks.

PulpFile Example Detail ViewSet:

class FileImporterViewSet(pulpcore.plugin.ImporterViewSet):
    @detail_route
    def sync(self):
        async_result = pulp_file.tasks.sync.apply_async_with_reservation(...)
        return OperationPostponed(async_result)

This will give the plugin writers the ability to add extra POST body arguments and perform synchronous validation. The task that is deployed is also written by the plugin writer, so they have absolute control over the entire life of the request. This is what is already expected for features like complex copy.

The sync logic on the plugins would be moved from the Detail Importer to a task, but would stay relatively the same.

@shared_task(base=UserFacingTask)
def sync(importer_pk):
    importer = models.FileImporter.objects.get(importer_pk) # Note, no cast() necessary the plugin knows which type
    synchronizer = Synchronizer(old_version, new_version).run()

Currently, working directory is set by the generic task, but this could be moved to a base task, WorkingDirectoryTask which would be part of the plugin api.

This change would result in the following REST API changes:

POST /v3/importers/file/123456789/sync/ repository=repository_href
POST /v3/publishers/file/123456789/publish/ repository_version=repository_version

Another benefit of having the plugin viewsets call plugin tasks is that the pattern is consistent and expandable. The plugin's sync route calls the plugin sync task, the plugin's rich copy route calls a rich copy task etc. This gives plugin writers one pattern to follow, which would be reinforced by the app's architecture:

pulp_plugin/
    app/
        models.py
        viewsets.py
        tasks.py

This also restricts RepositoryVersion to be a "manual" addition and removal of specified ContentUnits, which is consistent with all of our other DRF ViewSets, which are only CRUD without complex custom actions. Anything that is not CRUD would be implemented separately at action endpoints, keeping concerns separate.