OpenStack set default api version #5297
Conversation
For providers without api version, we take default v2. Otherwise UI shows blank field that is not an option Partial fix BZ https://bugzilla.redhat.com/show_bug.cgi?id=1278036
|
before: after: |
|
Checked commit Ladas@2ae4909 with ruby 1.9.3, rubocop 0.34.2, and haml-lint 0.13.0 |
| @@ -160,7 +160,7 @@ def ems_cloud_form_fields | |||
| :provider_id => @ems.provider_id ? @ems.provider_id : "", | |||
| :hostname => @ems.hostname, | |||
| :api_port => @ems.port, | |||
| :api_version => @ems.api_version, | |||
| :api_version => @ems.api_version ? @ems.api_version : "v2", | |||
There was a problem hiding this comment.
Should this default just be in the model instead? I prefer not putting logic in the UI. You can probably leverage default_value_for :api_version, "v2" on the models, and then this Just Works ™️
There was a problem hiding this comment.
I think UI code would need to change quite a bit. Now for the new Manager, they don't have here the correct class, just regular CloudManager. So I cannot put the default value to the correct class, which is the Openstack::CloudManager.
Although with the angular form, I think it is not supported to have default value per provider, since it doesn't do any requests when changing the provider type, the logic is in JS
So not sure, should I put the default value on ManageIQ::Providers::CloudManager? It doesn't belong there, it doesn't belong here, hard choice. :-)
There was a problem hiding this comment.
Why don't they have the right class? That seems wrong in and of itself.
There was a problem hiding this comment.
Although with the angular form, I think it is not supported to have default value per provider, since it doesn't do any requests when changing the provider type, the logic is in JS
Ugh that's horrible...that means all the business logic for new objects (ANY new object) has to be in Angular as well? cc @matthewd @chessbyte @blomquisg
There was a problem hiding this comment.
From my testing the angular changes form just according to provider type string, which you change by changing the selectbox of provider type, which is not ideal, cause I wanted to use the 'supports_..' methods we have on models. Maybe it's work in progress?
They don't have the right class since first request doesn't have the provider type set and then the logic is in JS.
There was a problem hiding this comment.
So I'm a little confused...This code in question is in the Rails side and you have the @ems, so not sure where angular fits in. If you use default_value_for in the model for api_version, then this code should work as it was.
There was a problem hiding this comment.
@Fryguy right, but for new provider, it's just EmsCloud, cause you don't know the type yet. So first thing you do is pick the type in selectbox, but it doesn't shoot request back to this code. I think it was like this in the non angular version?
So should I define the default value on EmsCloud (meaning ManageIQ::Providers::CloudManager) class?
There was a problem hiding this comment.
Yeah, that just feels wrong. I think if you pick the dropdown, then after that it should do a EmsThatSubclass.new.
There was a problem hiding this comment.
@Fryguy Yeah I agree, it should go back to this code when switching ems type, doesn't make sense to do the businesses logic in JS, for infra it will actually have to, since we have e.g. different selectboxes values for different EMS (unless we will preload all data for all EMSes. which again seems wrong).
Though that is currently above my angular skills :-D So will this suffice as a bacportable fix? While UI team should do later refactoring of the angular form?
There was a problem hiding this comment.
This is starting to sound like a larger change than this PR needs right now. Right?
I think we can merge what @Ladas has here, and then separately figure out if we're going off the, uh, rails with some of the form designs.
…_in_cloud_form OpenStack set default api version
…_in_cloud_form OpenStack set default api version
Bz1278036 Missing API version select box in cloud provider Clean cherry pick of: ManageIQ#5277 ManageIQ#5297 Fixed BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1278036 See merge request !428
For providers without api version, we take default v2. Otherwise
UI shows blank field that is not an option
Partial fix BZ
https://bugzilla.redhat.com/show_bug.cgi?id=1278036