Bug #11708
closed
New status code broke discovery plugin API
Description
I see no reason why to have all status-related methods in Host::Managed. This prevents plugins from using this in their owh STI sub-classes (e.g. discovery) and essentially breaks all API operations (patch is required: https://gist.github.com/lzap/2b1f83d2d47836fa9816).
Either move it to Host::Base or at least create a module that can be mixed into subclasses.
Updated by Lukas Zapletal over 9 years ago
- Related to Feature #10782: Add global status for hosts added
Updated by Dominic Cleal over 9 years ago
- Translation missing: en.field_release set to 63
Regarding your suggested rabl patch, why is Foreman's own host rabl being used for Host::Discovered? It's specifically for Host::Managed, since it's for Foreman's HostsController and shouldn't be used for other things that aren't at least sub-classes.
You could move parts of the status API to base if it'd be useful I guess. Probably the HostStatus subclasses we already have would only be linked to Host::Managed though, and only global status would be on base?
Updated by Dominic Cleal over 9 years ago
- Assignee deleted (
Marek Hulán)
https://github.com/theforeman/foreman_discovery/blob/cd560e59/app/views/api/v2/discovered_hosts/auto_provision.json.rabl looks a bit strange to me given that the broken test (http://ci.theforeman.org/job/test_plugin_matrix/357/database=mysql,label=fast,ruby=1.9.3/testReport/junit/%28root%29/Api__V2__DiscoveredHostsControllerTest/test_auto_provision_success/) calls the API to convert a Host::Discovered into a Host::Managed.
I think it's using the discovered host object from before it was converted to render the view, when it should be using the converted host after perform_auto_provision has been called in the controller. Try keeping hold of the host from that method (which will be Host::Managed), storing it in a controller instance variable and render the view using that instead.
The rest of the API looks correct, you're not using Host::Managed rabl views to render Host::Discovered generally as I suggested, so apologies - it's just that single action is passing in the wrong object to the Host::Managed view. It probably only passed tests before because rabl ignores errors for unknown attributes, but I don't think it would have been rendering the proper object.
Updated by Dominic Cleal over 9 years ago
- Project changed from Foreman to Discovery
- Category changed from Reporting to Discovery plugin
- Translation missing: en.field_release deleted (
63)
I'll move this over to Discovery as I think it should be fixed just in the API controller per my last comment.
If you're interested in having some status support on Host::Base then perhaps file another issue (including to say which bits of it are applicable), but I don't think it's necessary to fix the initial bug report.
Updated by
Updated by Lukas Zapletal over 9 years ago
I'd like to have DiscoveredStatus of course, it is interesting feature as we have several statuses currently. I will file another one then.
Updated by The Foreman Bot over 9 years ago
- Status changed from New to Ready For Testing
- Pull request https://github.com/theforeman/foreman_discovery/pull/214 added
- Pull request deleted (
)
Updated by Anonymous over 9 years ago
- Status changed from Ready For Testing to Closed
- % Done changed from 0 to 100
Applied in changeset foreman_discovery|3e16cea75005d6138ba22b95f219ad35fb0c7970.