fixes #8491 - removing puppet modules list from content view version api #4833
Conversation
| @@ -62,10 +63,3 @@ end | |||
| child :active_history => :active_history do | |||
| extends 'katello/api/v2/content_view_histories/show' | |||
| end | |||
|
|
|||
| child :puppet_modules => :puppet_modules do | |||
There was a problem hiding this comment.
I am not aware of us having a set in stone API policy, but this does introduce a change from 2.0 to 2.1 in the API response for this particular resource.
There was a problem hiding this comment.
It does introduce a minor change, but the performance at this level just is not acceptable. Changes that remove an entire api or remove an input parameter are easier to handle (as you can deprecate them and later remove). However removing a part of return data is much more difficult to do. The UI and Hammer all use the correct method for fetch puppet modules (the puppet modules controller), so i think its okay to introduce this change with a release note.
There was a problem hiding this comment.
It looks like in the CLI, the content view version info command shows puppet modules using this show template. I'm wondering if the better solution is to use foreman's model of having a shared template (main or base) that the show and index templates include. See operating systems for example:
https://github.com/theforeman/foreman/tree/develop/app/views/api/v2/operatingsystems
For index templates, they don't show any associations while in show templates they do. That seems to me to be a much better solution than having index templates simply include show templates. That said, I wouldn't necessarily be opposed to removing puppet_modules from index/show. We should at least update the CLI and remove puppet_modules from content view version info though.
There was a problem hiding this comment.
I'm wondering if the better solution is to use foreman's model of having a shared template (main) that the show and index templates include. See operating systems for example
Big 👍 to this. Having standard templates for resources makes this a lot easier, not even just index/show, but things like this too #4831
|
@daviddavis @stbenjam updated approach, let me know what you think |
|
👍 from me |
jlsherrill
changed the title
| @@ -35,7 +36,7 @@ def index | |||
| params[:sort_by] = 'version' | |||
| params[:sort_order] = 'desc' | |||
|
|
|||
| respond(:collection => collection) | |||
| respond(:collection => collection, :layout => 'index') | |||
There was a problem hiding this comment.
Are we not able to leave this up to the view?
There was a problem hiding this comment.
I don't believe a view can dictate what its layout is. The rendering helpers default to a 'collection' view, whereas this is overriding that. I think as we move to a 'base' rabl template we could default to an 'index' layout as well (and possibly get rid of those rendering helpers)
There was a problem hiding this comment.
What's the reasoning for moving away from just extending an index template in the view?
There was a problem hiding this comment.
The index template you refer to just extends the show template. This is a really bad design. See my discussion on foreman-dev ("API index templates in Katello") or #4833 (comment)
There was a problem hiding this comment.
There was a problem hiding this comment.
I think it means that he can't extend _metadata.json.rabl in the index layout. That means he either has to:
- Have content_view_versions/index.json.rabl extend
_metadata.json.rablandbase.json.rabl. - Not use
_metadata.json.rabland keep this PR the way it is.
I'm in favor of 2 because it's what foreman does and also _metadata.json.rabl will go away when we switch over to the index layout for our other index actions anyway. If we really wanted to, we could create a metadata layout I suppose.
There was a problem hiding this comment.
@ehelms any other thoughts?
There was a problem hiding this comment.
Gotcha. Can we just re-use their layout and override the functions we need to or just better to carry the logic ourselves?
There was a problem hiding this comment.
Honestly I don't think its worth it. we could use the layout and then either re-implement all of the helper functions or extend foreman's base helper and re-implement ones that would need to be different. Given the amount of 'logic' in the layout I really don't think using our own is a big deal. Hopefully the eventual move to v3 will give us the opportunity to centralize a lot of this logic in foreman.
…ersion api This takes the CVV index api from ~5 minutes down to 17 seconds for 47 content view versions, 10 having 100 puppet modules, the rest having 5
|
@daviddavis @ehelms ACK then? |
|
ACK from me |
|
ACK |
fixes #8491 - removing puppet modules list from content view version api
This takes the CVV index api from ~5 minutes down to 17 seconds
for 47 content view versions, 10 having 100 puppet modules, the rest having
5