Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate bindings for views/viewsets without model #577

Closed
wants to merge 1 commit into from

Conversation

fao89
Copy link
Member

@fao89 fao89 commented Mar 12, 2020

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

Please be sure you have read our documentation on creating PRs:
https://docs.pulpproject.org/contributing/pull-request-walkthrough.html

@fao89 fao89 marked this pull request as ready for review March 12, 2020 15:54
@fao89
Copy link
Member Author

fao89 commented Mar 16, 2020

@bmbouter @cutwater does it help with galaxy_ng?

@@ -0,0 +1 @@
Generate bindings for views/viewsets without model
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be paste tense with the new changelog style we are using?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the issue 5238 is for pulp_ansible, but this change is in core. Since pulpcore releases seperately from pulp_ansible, we end up in a strange position where we can't move it to CLOSED - CURRENT RELEASE unless both projects release. For this reason I avoid having pulpcore issues refer to any issue that isn't in the "core project" in Redmine.

I you agree w/ this, my suggestion is to file a new issue to track the defect that was in pulpcore specifically, and have your issue reference, refer to that one instead. Then relate the two issues in Redmine also.

Wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I will file an issue

@fao89 fao89 force-pushed the 5238 branch 3 times, most recently from 27a91fd to 9269111 Compare March 20, 2020 14:58
@@ -374,9 +402,11 @@ def get_tags(self, operation_keys):
list[str] of tags
"""
tags = self.overrides.get('tags')
full_tag = getattr(self.view, "pulp_full_tag", False)
Copy link
Member Author

@fao89 fao89 Mar 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tags are the things that identify the API, grouping it.
Tags are what you see on the left column: https://pulp-ansible.readthedocs.io/en/latest/restapi.html
They are formed by the URL:
/pulp_ansible/galaxy/{path}/api/v2/collections/ == Pulp_Ansible: Galaxy Api V2 Collections
On bindings:
Pulp_Ansible: Galaxy Api V2 Collections == class PulpAnsibleGalaxyApiV2CollectionsApi
As you can see below, we are deleting some word from the tag, which in some cases deleted v2 or v3, gathering everything in one API. This full_tag is for assuring galaxy APIs won't have any word deleted

resource_name = view.__name__.replace("ViewSet", "").replace("View", "")
param_name = path
resource_model = get_model_from_view(view)
slug_model = getattr(view, "pulp_slug", True)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pulp_slug was introduced to check if we want the current path URL or if we want a model URL e.g. pulp_href.
For CollectionImport:
pulp_slug = True => collection_import_href
pulp_slug = False => pulp_ansible/galaxy/{path}/api/v2/collection-imports/{task}/
In this specific case, as collection_import_href points to v3. It is a trick to differentiate v2 and v3 as they use the same viewset:
views_v3.CollectionImportViewSet: https://github.com/pulp/pulp_ansible/blob/master/pulp_ansible/app/urls.py#L36

@pulpbot
Copy link
Member

pulpbot commented Apr 3, 2020

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

@bmbouter
Copy link
Member

bmbouter commented May 1, 2020

This PR is important to me, but I likely need until next Wednesday before I can look at it. Sorry for the delay. I have a reminder, but please remind me again if that is ok.

@fao89
Copy link
Member Author

fao89 commented May 15, 2020

I rebased the PR and now it is failing due certguard

@bmbouter
Copy link
Member

If you rerun it now it should pass I believe. The failing test should be skipping now temporarily.

@fao89
Copy link
Member Author

fao89 commented May 26, 2020

Closing in favor of:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants