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 almost 10 years ago
- Related to Feature #10782: Add global status for hosts added
Updated by Dominic Cleal almost 10 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 almost 10 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 almost 10 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 almost 10 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 almost 10 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 almost 10 years ago
- Status changed from Ready For Testing to Closed
- % Done changed from 0 to 100
Applied in changeset foreman_discovery|3e16cea75005d6138ba22b95f219ad35fb0c7970.