fixes #3491 - API v2 rabl templates base, main, show for each controller #1062
Conversation
|
@tstrachota, I completed all the controllers. |
|
Where will base be used? |
|
@domcleal, ping? |
|
Is index for host not extending main or base intentionally? If I would like to add IP, environment and model to index to match UI, where should it go? Could you please explain a bit why is host attrs divided this way? |
|
I tend to agree, at least the attributes that directly assigned to the host (such as ipaddress) will not impact performance and would be very useful. |
|
Why audits and plugins do not follow this convention? Other than that I like it. At first I was hesitant about the main and base separation, but the use of base in nested resources makes a lot of sense. The views look nice and consistent this way. |
|
@mbacovsky, yes I have been meaning to add the host's attributes to be more comprehensive |
|
@isratrade cool, thanks! |
|
Some more comments:
|
| @@ -1,3 +1,3 @@ | |||
| object @architecture | |||
|
|
|||
| attributes :name, :id, :created_at, :operatingsystem_ids, :updated_at | |||
There was a problem hiding this comment.
lost operating systems? Shouldn't this be a child node?
There was a problem hiding this comment.
yes, has many attributes will be in PR #1062
| attributes :id, :login, :firstname, :lastname, :mail, :admin, :auth_source_id, :auth_source_name, :last_login_on, | ||
| :domains_andor, :hostgroups_andor, :facts_andor, :filter_on_owner, :compute_resources_andor, | ||
| :created_at, :updated_at | ||
| extends "api/v2/users/main" | ||
|
|
||
| child :auth_source do | ||
| extends "api/v2/auth_source_ldaps/show" |
|
Broadly looks good, thanks. Some things that came up a few times in my comments were:
|
|
@domcleal, I was just having this PR be adding the base, main, show templates and then clean up which attributes are in which file and DRY up things in another PR, but I will do it in this PR. |
|
@isratrade ok, I think we should have a go at getting it right now, hopefully most are quick fixes anyway |
|
@domcleal, re-committed as a separate commit to squash after review. |
|
@domcleal, it's much cleaner now. Thanks for your thorough review. +1 |
|
@mbacovsky, I moved |
|
@mbacovsky, can you explain what you mean by your comment above |
|
@mbacovsky, I added a new commit for audits/base and main. |
|
@mbacovsky, is there anything I read to change with api/plugins. I think I may have squashed and fixed this already. |
|
Oh, maybe plugins doesn't need to change as the controller only supports index. |
|
👍 |
|
I agree with @mbacovsky, this would be very useful for future reviews, though I'm going to merge this as-is for now. Thanks for the extra pair of eyes! Merged as 2e46934 for Foreman 1.4.0, thanks @isratrade. |
|
@mbacovsky, I agree with re-factoring in the future in the make the rabl files easier to read. |
base.json.rabl - usually contains
:id, :nameand possibly another important attributemain.json.rabl - extends 'base' and adds attributes, but no :child nodes
- index.json.rabl extends the 'main' template
show.json.rabl - extends 'main' and adds :child nodes
- currently, 'show' may not have anything different other than extending 'main', but this will probably change in PR
fixes #3492 to add nested routes and relationships in responses.