Skip to content
This repository has been archived by the owner on Oct 28, 2019. It is now read-only.

Add ReadOnlyContentViewset #140

Merged
merged 1 commit into from Oct 3, 2019
Merged

Add ReadOnlyContentViewset #140

merged 1 commit into from Oct 3, 2019

Conversation

goosemania
Copy link
Member

@@ -0,0 +1,2 @@
Add a ReadOnlyContentViewSet for plugin wirters.

Copy link
Member

Choose a reason for hiding this comment

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

I think this will look visually inconsistent in the rendered changelog. I recommend removing the extra line.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I can remove it. Sometimes it complains about lack of newline and I merge PRs in pulp_rpm with a newline and I don't think it affected release notes. No problem to remove it.

@@ -12,6 +12,7 @@
NamedModelViewSet,
Copy link
Member

Choose a reason for hiding this comment

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

Do we still want NamedModelViewSet in the plugin API?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would assume so. It's the base viewset for models and is being used in plugins like pulp_docker and pulp_ansible.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think so. What if it's not content but some other very custom viewset.

Copy link
Member

Choose a reason for hiding this comment

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

This sounds good. +1 Thank you.

@@ -12,6 +12,7 @@
NamedModelViewSet,
PublicationViewSet,
PublisherViewSet,
ReadOnlyContentViewSet,
Copy link
Member

Choose a reason for hiding this comment

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

Do we also want to add BaseContentViewSet so plugin writer's can use that as an equivalent to the "generic" use case?

Copy link
Contributor

Choose a reason for hiding this comment

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

I could go either way although my rule of thumb is to never add anything unless someone needs it.

Copy link
Member Author

Choose a reason for hiding this comment

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

My thinking is that we don't add it to the plugin API until we needed. I'm not against adding it right away if you think it's useful to do it now.

Copy link
Member

Choose a reason for hiding this comment

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

This works for me. Let's add it later.

Copy link
Member

@bmbouter bmbouter left a comment

Choose a reason for hiding this comment

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

This looks good to me, thank you!

@goosemania goosemania merged commit 7eded69 into pulp:master Oct 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants