fixes #9218 - extend hostgroups controller create/update to include katello attrs

[bbuckingham]

This commit adds an extension to the foreman hostgroups controller to
include katello attributes (content source, content view and
lifecyle environment). These attributes can currently be set
using the UI; however, we also need them in the API.

This is needed to support the fusor project.

[stbenjam]

Looks/works fine, and I know there's no other way to do this except redocument the entire API, but I worry this is too fragile.

In foreman_salt, I want to be able to extend hostgroups as well to take my parameters, and I can see other plugins wanting the same thing. It will be totally random who wins, right? Is there any plan to extend apipie to allow extending documentation (@iNecas?)?

The only other option at the moment is to have a separate Katello API controller for managing katello-specific hostgroup parameters, which is also really really less than ideal, but it is not fragile.

[stbenjam]

Actually, I think there is a solution...and we would need to contribute this to Foreman. I think Hosts and Host Groups should call some method from the respective model to find out what attributes to document.

e.g. Foreman's hostgroup and hosts controller would look like:

param :hostgroup, Hash :required => true, :action_aware true do
  param :name, String, :required => true
  param :parent_id, :number
  [etc, etc, etc]
  Hostgroup.plugin_attributes.each do |attribute|
    param attribute[:name], attribute[:options]
  end
end

and we'd alias_method_chain plugin_attributes or something. @domcleal? @elobato? Do you think you would accept something like this?

[bbuckingham]

@stbenjam, Thanks for looking and for the feedback! I totally agree that this can be a brittle solution; however, the current options appeared limited. Yesterday, I did raise an email on the theforeman-dev to inquire about options for apipie; however, I went ahead with this PR as I need the ability to assign these attributes using API in the near-term. I did consider using a separate controller; however, after discussion with @jlsherrill I did not pursue it further, as that seems even worse as it as implications on routes, cli...etc.

[dLobatog]

@stbenjam I think the snippet above should be something provided by apipie-rails natively. It seems to me that plugin-handling code has been a source of bugs in the past and the less code we have for that the better (without compromising extensibility).

Perhaps there are other options of doing this without having to touch Foreman core, @iNecas ?

[stbenjam]

Even if apipie has a way (AFAICS it doesn't), that doesn't deal with getting the attributes in the RABL. IMHO, stable plugin API is better than Rails hacks even if it's hard to get right. Nearly every plugin wants to add something to hosts and host groups.

[bbuckingham]

@stbenjam, We need to have the ability to set these attributes from the API. I realize this is not the ideal solution; however, could I get an ACK? I can create a redmine issue for apipie to request enhancements to hopefully allow us to remove this later.

[stbenjam]

If we can't find a solution that can be done for Foreman 1.8, I'll ACK it.

I would like a better answer from Foreman about how to extend their API's. ACK'ing this is taking on more technical debt 💰 we have to address later. And if any other plugin ever writes a concern to extend the hostgroups API, it will be totally random who wins...

[bbuckingham]

@stbenjam, Keep in mind, I need this as part of the current downstream release and I would prefer not to have to do a downstream only change.

[stbenjam]

But does this address the downstream issue? As the PR is written, the RABL is not extended, so its really only half a solution...

[bbuckingham]

@stbenjam, It addresses the need that I have. The other have could even be done as a separate PR.

[stbenjam]

If my proposal were used, this PR would look like this stbenjam@6d5c1a9, much simpler...

This is the Foreman code required: stbenjam/foreman@ffc9009 (actually works, but maybe could be DRYer or more Railsy)

Any plugin can extend hostgroup or hostgroup with whatever they wanted. It ends up in the apidoc, in the RABL, and, with some small changes, in hammer. Fixing apipie alone wouldn't get us that.

[bbuckingham]

@stbenjam, I like the concept of your proposal as it could also be useful to other plugins. Perhaps we can merge this PR (for the short-term) and get your proposed code PR'ed in to Foreman. Once it is accepted there, then we can easily refactor these changes.

[stbenjam]

Talked to @iNecas, he's extremely busy, and I doubt it's something we can get done in 1.8 time frame.
The foreman issue is http://projects.theforeman.org/issues/3763.

ACK

[iNecas]

Extension points in Apipie make sense and we can open an issue an discuss the format of that in the apipie repository.

However, since the apipie documentation itself is just a ruby code, one could do this even without any changes in the apipie itself. Let's say this was it the Foremans API documentation for hostgroup:

param :hostroup, Hash do
   param :name, String
   # ...
   Plugin.doc_extensions(:hostgroup).each { |block| instance_exec(&block) }
end
def create
end

And then having the foreman Plugin API ability to define the additional code blocks to be run at the particular space.

Adding something similar to the apipie itself (without even specifying the extension point explicitly) might actually not be that hard. A PR addressing this might be done in few days, with a new release of Apipie in a week or two: I still would like to give the Apipie community some place for feedback before getting it into new release).

[iNecas]

Btw. there are other things to be considered, than just the API documentation: with movement to the Rails 4, we will face the strong-parameters issues as well, it should be probably considered as well when comming up with some solution.

[stbenjam]

Yea, your example is basically the same as stbenjam/foreman@ffc9009 although I don't think I'd want to feed it a block, but rather individual params so they can be re-used in the RABL (and I guess allowing mass assignment to them in the model). I don't even know that Apipie really needs to do anything, I think this works fine because we have to think about more than just documentation...

[iNecas]

Here is an issue to track against apipie Apipie/apipie-rails#330, @stbenjam I like your approach A LOT!