Remove features deprecated in 3.12 for 3.13 compatibility #563

dralley · 2021-04-19T01:36:30Z

closes: #8589
https://pulp.plan.io/issues/8589

pep8speaks · 2021-04-19T01:36:36Z

Hello @dralley! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-05-18 19:34:41 UTC

pulpbot · 2021-04-19T02:18:33Z

Attached issue: https://pulp.plan.io/issues/8589

dralley · 2021-04-22T00:15:52Z

The test failures are because of a loosened restriction - "repository" and "repository_version" are now allowed to be used together. @fao89 Does the ansible plugin want to enforce this? Depending on the use case I think it could be reasonable for some plugins to want to do that.

fao89 · 2021-04-22T14:42:41Z

The test failures are because of a loosened restriction - "repository" and "repository_version" are now allowed to be used together. @fao89 Does the ansible plugin want to enforce this? Depending on the use case I think it could be reasonable for some plugins to want to do that.

I can't think of a reason to enforce it, currently, we already try both https://github.com/pulp/pulp_ansible/blob/master/pulp_ansible/app/galaxy/v3/views.py#L57-L71

@daviddavis wdyt?

bmbouter · 2021-05-10T20:57:11Z

pulp_ansible/app/viewsets.py

            update_collection_remote,
            lock,
-            args=(pk,),
+            args=(str(pk),),


This str() can be removed I think.

bmbouter · 2021-05-10T20:57:48Z

pulp_ansible/app/viewsets.py

@@ -376,15 +380,15 @@ def _process_config(self, config):
                entry["source_repo_version"], RepositoryVersion
            )
            dest_repo = NamedModelViewSet().get_resource(entry["dest_repo"], AnsibleRepository)
-            r["source_repo_version"] = source_version.pk
-            r["dest_repo"] = dest_repo.pk
+            r["source_repo_version"] = str(source_version.pk)


All the str() call can be removed actually. The next release this code will go into (pulp_ansible 0.8) will be compatible with pulpcore 3.13 which no longer requires these.

bmbouter · 2021-05-10T21:27:37Z

Using master (without this PR) I created a distribution, and here is what the database master/detail shows:

pulp=> select * from core_basedistribution;
               pulp_id                |         pulp_created          |       pulp_last_updated       |    pulp_type    |       name        | base_path | content_guard_id | remote_id 
--------------------------------------+-------------------------------+-------------------------------+-----------------+-------------------+-----------+------------------+-----------
 4084b54c-82ab-4826-8d6c-f4e772192cab | 2021-05-10 21:22:30.251446+00 | 2021-05-10 21:22:30.251458+00 | ansible.ansible | distribution26149 | 17985     |                  | 
(1 row)

pulp=> select * from ansible_ansibledistribution;
       basedistribution_ptr_id        |            repository_id             | repository_version_id 
--------------------------------------+--------------------------------------+-----------------------
 4084b54c-82ab-4826-8d6c-f4e772192cab | c1858c7b-1715-429a-a0fd-c7b0ccdad0e1 | 
(1 row)

Then I upgrade to this PR, run the migrations and the db now looks like:

pulp=> select * from core_basedistribution;
 pulp_id | pulp_created | pulp_last_updated | pulp_type | name | base_path | content_guard_id | remote_id 
---------+--------------+-------------------+-----------+------+-----------+------------------+-----------
(0 rows)

pulp=> select * from core_distribution;
               pulp_id                |         pulp_created          |       pulp_last_updated       |    pulp_type    |       name        | base_path | content_guard_id | publication_id | remote_id |            repository_id             | repository_version_id 
--------------------------------------+-------------------------------+-------------------------------+-----------------+-------------------+-----------+------------------+----------------+-----------+--------------------------------------+-----------------------
 4084b54c-82ab-4826-8d6c-f4e772192cab | 2021-05-10 21:24:50.448434+00 | 2021-05-10 21:24:50.448456+00 | ansible.ansible | distribution26149 | 17985     |                  |                |           | c1858c7b-1715-429a-a0fd-c7b0ccdad0e1 | 
(1 row)

pulp=> select * from ansible_ansibledistribution;
         distribution_ptr_id          
--------------------------------------
 4084b54c-82ab-4826-8d6c-f4e772192cab
(1 row)

So that all looks successful!

Also when starting the upgraded ansible, the CLI continues to list the distribution correctly:

(pulp) [vagrant@pulp3-source-fedora33 ~]$ pulp ansible distribution list
[
  {
    "pulp_href": "/pulp/api/v3/distributions/ansible/ansible/4084b54c-82ab-4826-8d6c-f4e772192cab/",
    "pulp_created": "2021-05-10T21:24:50.448434Z",
    "base_path": "17985",
    "content_guard": null,
    "name": "distribution26149",
    "repository": "/pulp/api/v3/repositories/ansible/ansible/c1858c7b-1715-429a-a0fd-c7b0ccdad0e1/",
    "repository_version": null,
    "client_url": "http://pulp3-source-fedora33.localhost.example.com/pulp_ansible/galaxy/17985/",
    "pulp_labels": {}
  }
]

So functionally speaking, this is all looking great.

daviddavis · 2021-05-11T12:14:36Z

I can't think of a reason to enforce it, currently, we already try both https://github.com/pulp/pulp_ansible/blob/master/pulp_ansible/app/galaxy/v3/views.py#L57-L71

Apologies for not seeing this sooner. My understanding (which is perhaps outdated) is that you can either distribute the repository (which distributes the latest version) or a particular repo version. So I'm not sure I understand the use case where a user would set both?

bmbouter · 2021-05-11T14:28:56Z

I can't think of a reason to enforce it, currently, we already try both https://github.com/pulp/pulp_ansible/blob/master/pulp_ansible/app/galaxy/v3/views.py#L57-L71

Apologies for not seeing this sooner. My understanding (which is perhaps outdated) is that you can either distribute the repository (which distributes the latest version) or a particular repo version. So I'm not sure I understand the use case where a user would set both?

This is also my understanding.

dralley · 2021-05-12T03:48:54Z

@daviddavis @bmbouter

The semantics on publication-based plugins are:

Set repository if you want the distribution to be automatically updated with the latest publications for that repo (updates are pushed to publication)
Set publication if you want to temporarily override that (or initially set it) with a particular publication, or if you aren't setting repository, this value never changes

The semantics on repository-version based plugins are:

Set repository if you want the distribution to automatically fetch the latest repository version for that repo (the update is pulled), OR
Set repository_version, which never changes.

What I'm asking is, in effect, if we want to keep these separate semantics, or if we want to make plugins like ansible use the same semantics as publication-based plugins, in which case it having both unlocked at the same time makes sense.

bmbouter · 2021-05-12T13:46:13Z

@dralley I'm not fully understanding the suggestion. I can tell I'm not because I don't see an opportunity to change the semantics. Right now I think since the detail serializer doesn't have a publication field so I'm thinking all we can do is to keep the semantics of repository-version based plugins. At some point in the future we will probably make pulp_ansible a publication-based plugin, but at that time we can adjust the serializers, introduce the right fields and switch the semantics. Is this helpful to give you the answer, or did I miss it?

dralley · 2021-05-12T14:17:10Z

@bmbouter I'm not suggesting using publications, I'm suggesting using the same behavior.

i.e. New version gets created, if repository is set, we update repository_version on the distributions.

bmbouter · 2021-05-18T15:44:40Z

I think this is waiting on pulpcore PR, is that right? If so, which one?

bmbouter · 2021-05-18T18:24:22Z

I was told on IRC this was waiting on pulp/pulpcore#1335

closes: #8589 https://pulp.plan.io/issues/8589

bmbouter · 2021-05-18T20:25:18Z

pulp_ansible/app/tasks/collections.py

+        msg = _("no_change: Checking if remote changed since last sync.")
+        noop = ProgressReport(message=msg, code="sync.no_change")


I didn't expect this, but I'm ok with it.

As an explanation, since more than one logical "task" could be going on within a literal "task", we've been updating progress reports to be specific about what progress they are reporting.

A publish can now happen in the same task as a sync, so therefore we want to make the progress reporting conventions more specific.

But you're right, that isn't clear from just the PR description and the issue. Apologies, this is wrapping up a few different things at once.

It's no problem. Thank you @dralley !

bmbouter · 2021-05-18T20:25:28Z

pulp_ansible/app/tasks/roles.py

@@ -50,7 +50,7 @@ def synchronize(remote_pk, repository_pk, mirror=False):
    )
    first_stage = RoleFirstStage(remote)
    d_version = DeclarativeVersion(first_stage, repository, mirror=mirror)
-    d_version.create()
+    return d_version.create()


I didn't expect this, but I'm ok with it.

Same here. It doesn't change anything since we're not using this result, but the convention is that we could be using this result.

bmbouter · 2021-05-18T20:25:51Z

pulp_ansible/app/tasks/roles.py

@@ -76,7 +76,7 @@ async def run(self):
        """
        Build and emit `DeclarativeContent` from the ansible metadata.
        """
-        with ProgressReport(message="Parsing Role Metadata", code="parsing.metadata") as pb:
+        with ProgressReport(message="Parsing Role Metadata", code="sync.parsing.metadata") as pb:


also this, but I'm ok with it.

bmbouter

Thank you @dralley !

fao89 · 2021-05-18T20:29:01Z

This change is compatible with 3.12? If so, I can do the release tomorrow!

bmbouter · 2021-05-18T20:29:49Z

This change is compatible with 3.12? If so, I can do the release tomorrow!

It is because the new Distribution object was introduced in 3.12

dralley · 2021-05-18T20:31:38Z

@fao89 Not 3.12.0, but yes 3.12.1. There was an oversight where we forgot to put RepositoryVersionRelatedField into the plugin API in 3.12.0.

dralley force-pushed the abstract-field-migration branch 3 times, most recently from b935eea to 51b8ae6 Compare April 19, 2021 01:47

dralley force-pushed the abstract-field-migration branch 3 times, most recently from 849c16e to f61cc95 Compare April 21, 2021 17:05

dralley marked this pull request as ready for review April 21, 2021 17:06

dralley requested a review from a team as a code owner April 21, 2021 17:06

dralley force-pushed the abstract-field-migration branch from 32aa98d to 748d174 Compare April 22, 2021 14:11

bmbouter reviewed May 10, 2021

View reviewed changes

dralley force-pushed the abstract-field-migration branch 3 times, most recently from 803915f to 5217936 Compare May 17, 2021 18:33

Remove features deprecated in 3.12 for 3.13 compatibility

9ac7d73

closes: #8589 https://pulp.plan.io/issues/8589

dralley force-pushed the abstract-field-migration branch from 5217936 to 9ac7d73 Compare May 18, 2021 19:34

bmbouter reviewed May 18, 2021

View reviewed changes

bmbouter approved these changes May 18, 2021

View reviewed changes

daviddavis approved these changes May 19, 2021

View reviewed changes

fao89 merged commit 4b3d63f into pulp:master May 19, 2021

dralley deleted the abstract-field-migration branch May 19, 2021 16:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove features deprecated in 3.12 for 3.13 compatibility #563

Remove features deprecated in 3.12 for 3.13 compatibility #563

dralley commented Apr 19, 2021

pep8speaks commented Apr 19, 2021 •

edited

Loading

pulpbot commented Apr 19, 2021

dralley commented Apr 22, 2021

fao89 commented Apr 22, 2021

bmbouter May 10, 2021

bmbouter May 10, 2021

bmbouter commented May 10, 2021

daviddavis commented May 11, 2021

bmbouter commented May 11, 2021

dralley commented May 12, 2021 •

edited

Loading

bmbouter commented May 12, 2021

dralley commented May 12, 2021

bmbouter commented May 18, 2021

bmbouter commented May 18, 2021

bmbouter May 18, 2021

dralley May 18, 2021

bmbouter May 18, 2021

bmbouter May 18, 2021

dralley May 18, 2021

bmbouter May 18, 2021

bmbouter left a comment

fao89 commented May 18, 2021

bmbouter commented May 18, 2021

dralley commented May 18, 2021

		msg = _("no_change: Checking if remote changed since last sync.")
		noop = ProgressReport(message=msg, code="sync.no_change")

Remove features deprecated in 3.12 for 3.13 compatibility #563

Remove features deprecated in 3.12 for 3.13 compatibility #563

Conversation

dralley commented Apr 19, 2021

pep8speaks commented Apr 19, 2021 • edited Loading

Comment last updated at 2021-05-18 19:34:41 UTC

pulpbot commented Apr 19, 2021

dralley commented Apr 22, 2021

fao89 commented Apr 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmbouter commented May 10, 2021

daviddavis commented May 11, 2021

bmbouter commented May 11, 2021

dralley commented May 12, 2021 • edited Loading

bmbouter commented May 12, 2021

dralley commented May 12, 2021

bmbouter commented May 18, 2021

bmbouter commented May 18, 2021

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

Choose a reason for hiding this comment

bmbouter left a comment

Choose a reason for hiding this comment

fao89 commented May 18, 2021

bmbouter commented May 18, 2021

dralley commented May 18, 2021

pep8speaks commented Apr 19, 2021 •

edited

Loading

dralley commented May 12, 2021 •

edited

Loading