Align importer models and serializers with MVP #3117

asmacdo · 2017-08-09T18:39:02Z

This commit aligns the Importer model and serializer with the MVP
document for Pulp 3. The model docstrings are redundant with the
serializer help_text. The help_text will eventually become the
documentation, so removing using it exclusively will be the best way to
keep the code, comments, and documentation up to date.

jortel · 2017-08-15T21:30:25Z

platform/pulpcore/app/models/repository.py

-        (BACKGROUND, 'Download In Background'))
+        (IMMEDIATE, 'Sync task is not complete until downloading is finished.'),
+        (ON_DEMAND, 'Sync task updates metadata but does not download content until requested.'),
+        (BACKGROUND, 'Sync task updates metadata and completes. Downloads complete in background.'))


Suggest something more like:

IMMEDIATE: Update the repository content and ensures all artifacts are downloaded.

ON_DEMAND: Update the repository content but no artifacts are downloaded.

BACKGROUND: Update the repository content and downloads artifacts in the background.

The tasking seems irrelevant to the definition.

jortel · 2017-08-15T21:31:51Z

platform/pulpcore/app/models/repository.py

-
-    Relations:
-
-        scratchpad (GenericKeyValueRelation): Arbitrary information stashed by the importer.


I'm not sold on removing the docstrings. Also, it seems outside the scope of this issue.

jortel · 2017-08-15T21:35:55Z

platform/pulpcore/app/models/repository.py

+    MIRROR = 'mirror'
+    SYNC_MODES = (
+        (ADDITIVE, 'Add new content, keep content that was removed upstream.'),
+        (MIRROR, 'Add new content, remove content was removed upstream.'))


Suggest something like:

ADDITIVE: Add new content.

MIRROR: Add new content and remove content that is no longer in the remote repository.

For additive, some content could still be removed from the repository in cases such as RPM only keeping X versions of RPMs.

jortel · 2017-08-15T21:40:26Z

platform/pulpcore/app/serializers/repository.py

+        choices=models.Importer.DOWNLOAD_POLICIES,
+    )
+    sync_mode = serializers.ChoiceField(
+        help_text='How the importer should sync from the upstream repository.',


I quess I lost the battle on pulp-dev for sync vs synchronize but really don't see value in using this abbreviation. Is it really that long a word to type?

Sync vs synchronize is often the difference between 1 and 2 lines. Obviously, this is a minor concern, but single lines are much easier to scan visually, and I think it has a noticeable impact on readability. As it turns out, sync is actually a real word, [informal]. http://www.dictionary.com/browse/sync

jortel · 2017-08-15T21:40:56Z

platform/pulpcore/app/serializers/repository.py

    validate = serializers.BooleanField(
-        help_text='Whether to validate imported content.',
+        help_text='If True, the plugin will validate imported content.',


To be clear, the content is not validated, the downloaded artifacts are.

jortel · 2017-08-15T21:42:18Z

platform/pulpcore/app/serializers/repository.py

        required=False,
    )
-
    ssl_ca_certificate = FileField(
        help_text='A PEM encoded CA certificate used to validate the server '
                  'certificate presented by the external source.',


Suggest removing: presented by the external source and say remote server instead.

jortel · 2017-08-15T21:46:15Z

platform/pulpcore/app/serializers/repository.py

    last_sync = serializers.DateTimeField(
        help_text='Timestamp of the most recent successful sync.',
        read_only=True
    )
+    last_updated = serializers.DateTimeField(
+        help_text='Timestamp of the most recent update of the importer configuration.',


The importer fields are the configuration. Suggest removing: configuration.

jortel · 2017-08-15T21:54:37Z

platform/pulpcore/app/serializers/repository.py

            'ssl_client_certificate', 'ssl_client_key', 'ssl_validation', 'proxy_url',
-            'basic_auth_user', 'basic_auth_password', 'download_policy', 'last_sync', 'repository',
+            'username', 'password', 'last_sync', 'last_updated', 'repository',


Large paragraph style lists are, IMHO, less readable. consider:

...Meta.fields + ( 'name', 'feed_url', ... )

Also, easier to keep alphabetized (if so inclined).

Sorry, in the mood to be pedantic.

jortel · 2017-08-22T15:27:03Z

@asmacdo: These changes will likely break the FileImporter. Please ensure a file plugin PR can be merged at the same time.

jortel · 2017-09-14T21:30:31Z

platform/pulpcore/app/models/repository.py

@@ -97,23 +97,24 @@ class Importer(ContentAdaptor):
    Fields:

        feed_url (models.TextField): The URL of an external content source.
-        validate (models.BooleanField): Validate the imported context.
+        validate (models.BooleanField): If True, the plugin will validate imported files.


s/imported/downloaded/ might be clearer.

Is this also used for uploads?

@dkliban, I am going to leave this for today and merge. If uploads don't use this field, maybe we should specify that it is only for downloads.

jortel · 2017-09-14T21:34:04Z

platform/pulpcore/app/models/repository.py


    download_policy = models.TextField(choices=DOWNLOAD_POLICIES)
-    last_sync = models.DateTimeField(blank=True, null=True)
+    sync_mode = models.TextField(choices=SYNC_MODES)
+    last_synced = models.DateTimeField(blank=True, null=True)


Looks like the task is not updating last_synced. I'll write an issue.

jortel · 2017-09-14T21:35:33Z

platform/pulpcore/app/serializers/repository.py

    validate = serializers.BooleanField(
-        help_text='Whether to validate imported content.',
+        help_text='If True, the plugin will validate imported artifacts.',


s/imported/downloaded/

jortel

Left a few more minor suggestions.

closes pulp#2818 re pulp#2817 closes pulp#2816 https://pulp.plan.io/projects/pulp/wiki/Pulp_3_Minimum_Viable_Product

asmacdo changed the title ~~Align importers with~~ Align importer models and serializers with MVP Aug 9, 2017

jortel reviewed Aug 15, 2017

View reviewed changes

jortel added the 3.0 label Aug 16, 2017

asmacdo force-pushed the align-importer branch 4 times, most recently from 8b3fa0e to 1dac3c3 Compare September 14, 2017 21:27

jortel reviewed Sep 14, 2017

View reviewed changes

jortel approved these changes Sep 14, 2017

View reviewed changes

Align Importer fields and text with MVP document

9ad9669

closes pulp#2818 re pulp#2817 closes pulp#2816 https://pulp.plan.io/projects/pulp/wiki/Pulp_3_Minimum_Viable_Product

asmacdo force-pushed the align-importer branch from 1dac3c3 to 9ad9669 Compare September 14, 2017 21:44

asmacdo merged commit af0cc71 into pulp:3.0-dev Sep 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Align importer models and serializers with MVP #3117

Align importer models and serializers with MVP #3117

asmacdo commented Aug 9, 2017

jortel Aug 15, 2017

jortel Aug 15, 2017 •

edited

Loading

jortel Aug 15, 2017

jortel Aug 15, 2017

asmacdo Sep 14, 2017

jortel Aug 15, 2017

jortel Aug 15, 2017 •

edited

Loading

jortel Aug 15, 2017

jortel Aug 15, 2017

jortel commented Aug 22, 2017

jortel Sep 14, 2017

asmacdo Sep 14, 2017

jortel Sep 14, 2017

asmacdo Sep 14, 2017

jortel Sep 14, 2017 •

edited

Loading

jortel Sep 14, 2017

jortel left a comment


		Relations:

		scratchpad (GenericKeyValueRelation): Arbitrary information stashed by the importer.

Align importer models and serializers with MVP #3117

Align importer models and serializers with MVP #3117

Conversation

asmacdo commented Aug 9, 2017

Choose a reason for hiding this comment

jortel Aug 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jortel Aug 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jortel commented Aug 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jortel Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jortel left a comment

Choose a reason for hiding this comment

jortel Aug 15, 2017 •

edited

Loading

jortel Aug 15, 2017 •

edited

Loading

jortel Sep 14, 2017 •

edited

Loading