OPTIONS for /api/providers #3

enoodle · 2017-08-08T09:16:51Z

This is a migration of ManageIQ/manageiq#15660

enoodle · 2017-08-08T09:18:04Z

jntullo · 2017-08-08T12:42:09Z

app/controllers/api/providers_controller.rb

+                           .select { |param| param.starts_with?("ems_type") } # 'parameters with "ems_type" name
+                           .first.split('=').last                             # choosing only the first occuracne.
+      ems_class = ExtManagementSystem.descendants.select { |ems| ems.ems_type == req_class_name }.first
+      render :json => ems_class.respond_to?(:provider_settings) ? ems_class.provider_settings : {}


I think you will want to use render_options(:providers, <your provider settings hash) here. This will return all of the default options that are passed back from OPTIONS /providers, and append your provider settings hash to it.

@jntullo I changed to render_options

jntullo · 2017-08-08T14:52:43Z

app/controllers/api/providers_controller.rb

+                           .split('?').last.split('&')                        # '?' for query, '&' connects parameters
+                           .select { |param| param.starts_with?("ems_type") } # 'parameters with "ems_type" name
+                           .first.split('=').last                             # choosing only the first occuracne.
+      ems_class = ExtManagementSystem.descendants.select { |ems| ems.ems_type == req_class_name }.first


missed this earlier, but you should be able to get the ems type from just params[:ems_type]. Then you can actually use ExtManagementSystem.descendant_get(params[:ems_type]) here. That will return the descendant. An example of its usage can be found https://github.com/ManageIQ/manageiq-api/blob/master/lib/services/api/authentication_service.rb#L4

@jntullo I didn't know of descendant_get before, but I am not sure it will help here because it will require sending the full class name (ie ManageIQ::Providers::Openshift::ContainerManager) instead of the ems_type reported by the providers. I will change to use params[:ems_type]

@enoodle ah, gotcha! yeah, then descendant_get won't work.

this is just nit but you could do ExtManagementSystem.descendants.find { |ems| ems.ems_type == params[:ems_type] }

jntullo · 2017-08-08T15:26:12Z

spec/requests/api/providers_spec.rb

+  context "#options" do
+    it "returns options for queried provider class" do
+      api_basic_authorize
+      ExtManagementSystem.descendants.select { |ems| ems.respond_to?(:provider_settings) }.each do |ems_type|


one last thing 😄 you can simplify the spec by using expect_options_results, example here: https://github.com/ManageIQ/manageiq-api/blob/master/spec/requests/api/authentications_spec.rb#L480

imtayadeway · 2017-08-08T19:55:56Z

spec/requests/api/providers_spec.rb

+
+  context "#options" do
+    it "returns options for queried provider class" do
+      api_basic_authorize


Doing an OPTIONS request shouldn't require authorization

@imtayadeway I am not sure how to remove this requirement. I also see this in many other examples: cluster, hosts, authentications. How do you suggest solving this?

I didn't realize this was still an issue.....I created #8 to resolve this

imtayadeway · 2017-08-08T20:00:35Z

app/controllers/api/providers_controller.rb

@@ -85,6 +85,11 @@ def import_vm_resource(type, id = nil, data = {})
      end
    end

+    def options
+      ems_class = ExtManagementSystem.descendants.find { |ems| ems.ems_type == params[:ems_type] }
+      render_options(:providers, ems_class.respond_to?(:provider_settings) ? ems_class.provider_settings : {})


It looks like you don't render anything when you don't provide the ems_type param. Did you you consider rendering all the EMSs and their provider settings and leaving it to the client to figure out?

imtayadeway · 2017-08-08T20:04:01Z

spec/requests/api/providers_spec.rb

+  context "#options" do
+    it "returns options for queried provider class" do
+      api_basic_authorize
+      ExtManagementSystem.descendants.select { |ems| ems.respond_to?(:provider_settings) }.each do |ems_type|


Your test logic here mirrors that of the implementation, so I'm wondering what value this test is providing? IMO, using literals to represent expected values is much clearer and will provide useful feedback in the case of breakages or false assumptions about the implementations.

@imtayadeway doing so will force me to use specific providers names as this is only implemented for "ManageIQ::Providers::Kubernetes::ContainerManager" currently. I am afraid of breaking tests when things changes in another repo (ie someone will change the way options are saved there and tests here will be red). I also agree that the test logic is not super helpful but it helped me.
I will try to add a test with this provider and options that I don't think will change anytime soon.

imtayadeway · 2017-08-09T15:09:09Z

app/controllers/api/providers_controller.rb

@@ -85,6 +85,21 @@ def import_vm_resource(type, id = nil, data = {})
      end
    end

+    def options
+      if params[:ems_type].blank?


I still have concerns over this param and its discoverability. At present AFAICT it is not discoverable, meaning it will have to be documented somewhere, and I'm not totally comfortable with adding complexity to OPTIONS calls. Is it needed?

It is not a must, but I though it will be nice. I don't mind documenting it (where?) but I can also remove this.

See also #3 (comment) for my explanation there - I think I'd prefer not including this unless there is a distinct need for it. If that is the case, I suspect OPTIONS might not be the right solution

imtayadeway · 2017-08-09T15:13:41Z

app/controllers/api/providers_controller.rb

+      if params[:ems_type].blank?
+        all_options = Hash[
+          ExtManagementSystem.descendants.collect do |ems|
+            [ems.ems_type, ems.respond_to?(:provider_settings) ? ems.provider_settings : {}]


Do provider settings come from the Settings object? Could this information be got at through the settings API?

No, they come from "per-provider" options, for example: ManageIQ/manageiq-providers-kubernetes#45

Is there a reason that's not in here ?

Yes, those options themselves are per provider instance.
What I am serving here are only their descriptions but belong there as they are static and not configurable.

I'm not sure I follow? I'd expect per-instance settings to be revealed via GETs to the collection member URI. We augment OPTIONS responses only as a limited way to reveal metadata - i.e. data about the resource itself in the abstract.

Right. But here we are not serving the values of those options but the answer to the question "what per-provider options are for this provider type?". This is the static description of those options. please look at https://github.com/ManageIQ/manageiq-providers-kubernetes/pull/45/files#diff-47711b761083b6ce40dacb7d3ee68f8b for example

imtayadeway · 2017-08-09T16:38:28Z

spec/requests/providers_spec.rb

+
+  context "OPTIONS /api/providers" do
+    it "returns options for all providers when no query" do
+      api_basic_authorize


Now that #8 has been merged you can remove this and subsequent calls to api_basic_authorize

imtayadeway · 2017-08-09T18:50:52Z

app/controllers/api/providers_controller.rb

@@ -85,6 +85,16 @@ def import_vm_resource(type, id = nil, data = {})
      end
    end

+    def options
+      all_options = Hash[
+        ExtManagementSystem.descendants.collect do |ems|


Prefer inject or each_with_object here

imtayadeway · 2017-08-09T19:03:05Z

app/controllers/api/providers_controller.rb

+          [ems.ems_type, ems.respond_to?(:provider_settings) ? ems.provider_settings : {}]
+        end
+      ]
+      all_options.delete(nil)


Which EMSs have an ems_type of nil?

ExtManagementSystem.descendants.select { |ems| ems.ems_type == nil }.map { |ems| ems.name } => ["ManageIQ::Providers::BaseManager", "ManageIQ::Providers::DatawarehouseManager", "ManageIQ::Providers::MiddlewareManager", "ManageIQ::Providers::NetworkManager", "ManageIQ::Providers::ProvisioningManager", "ManageIQ::Providers::AutomationManager", "ManageIQ::Providers::CloudManager", "ManageIQ::Providers::ContainerManager", "ManageIQ::Providers::ConfigurationManager", "ManageIQ::Providers::StorageManager", "ManageIQ::Providers::InfraManager", "ManageIQ::Providers::EmbeddedAutomationManager", "ManageIQ::Providers::ExternalAutomationManager"]

Will .leaf_subclasses give you what you need?

imtayadeway · 2017-08-09T20:45:03Z

@enoodle just one more suggestion in #3 (comment). Thanks!

enoodle · 2017-08-10T14:26:11Z

@imtayadeway PTAL

imtayadeway · 2017-08-10T14:52:30Z

app/controllers/api/providers_controller.rb

@@ -85,6 +85,13 @@ def import_vm_resource(type, id = nil, data = {})
      end
    end

+    def options
+      all_options = ExtManagementSystem.leaf_subclasses.inject({}) do |ao, ems|
+        ao.merge(ems.ems_type => ems.respond_to?(:provider_settings) ? ems.provider_settings : {})


It's unfortunate that not all EMSs respond to this message. Though I think this if fine for now - do you think you could follow this up by trying to push this default down into the base class?

Yes. I changed this to address leaf_subclasses of ManageIQ::Providers::BaseManager instead of ExtManagementSystem and it will be easy to add

def self.provider_settings() {} end

to this class.

ManageIQ/manageiq#15799

imtayadeway · 2017-08-10T14:55:09Z

spec/requests/providers_spec.rb

+    it "returns options for all providers when no query" do
+      run_options(providers_url)
+      expect(response.parsed_body["data"].keys.count).to eq(
+        ExtManagementSystem.leaf_subclasses.select { |ems| !ems.ems_type.blank? }.count


Are there any leaf subclasses for which this is not true?

No (that I know of), but if a new one will be created I think it will break this test. I will remove the select.

enoodle · 2017-08-14T12:38:47Z

@imtayadeway PTAL

imtayadeway

LGTM! Thanks @enoodle for your patience with my many questions.

Off to @abellotti

enoodle · 2017-08-15T09:04:08Z

@miq-bot add_label wip
We will change the provider_settings function name to options_description

enoodle · 2017-08-24T14:36:01Z

@imtayadeway @abellotti with ManageIQ/manageiq#15799 being approved, can we merge this one?

imtayadeway · 2017-08-24T14:38:05Z

app/controllers/api/providers_controller.rb

@@ -86,6 +86,13 @@ def import_vm_resource(type, id = nil, data = {})
      end
    end

+    def options
+      all_options = ManageIQ::Providers::BaseManager.leaf_subclasses.inject({}) do |ao, ems|
+        ao.merge(ems.ems_type => ems.respond_to?(:options_description) ? ems.options_description : {})


@enoodle if ManageIQ/manageiq#15799 is merged, can you remove this respond_to??

Changed, tests here will only be green after ManageIQ/manageiq#15799 is merged

abellotti · 2017-08-24T14:48:44Z

@enoodle can you attach sample output you're getting with OPTIONS /api/providers ? just the data part of the output is sufficient. Thanks.

enoodle · 2017-08-24T15:08:00Z

Output example from current masters:

api_provider_to_delete.txt

abellotti · 2017-08-24T15:21:07Z

I wonder if there might be some common options we'd want to provide in the future for all providers, in which case we may want to nest the output one level down,

i.e.

"data" => {
   "provider_settings" => {
       "ansible_tower_automation" => ...,
       "azure" => ...,
       ...
   }
}

enoodle · 2017-08-25T05:45:16Z

@abellotti Then you suggest to have the common options description under "provider_settings"? It will make for a more compact output, but will be more awkward to consume so I don't like it too much.

abellotti · 2017-08-30T16:03:49Z

@enoodle true, it is an extra level to fetch, but would keep the return signature future-proof.

enoodle · 2017-09-03T13:09:45Z

@abellotti 👍 I added the "provider_settings" step.

miq-bot · 2017-09-04T08:44:58Z

Checked commit enoodle@cb3b52c with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 1 offense detected

app/controllers/api/providers_controller.rb

❗ - Line 93, Col 34 - Style/BracesAroundHashParameters - Redundant curly braces around a hash parameter.

enoodle · 2017-09-04T09:10:10Z

Travis for this seems to be broken as it is testing the wrong commit. I will close this one to open a new PR.

jntullo suggested changes Aug 8, 2017

View reviewed changes

enoodle force-pushed the provider_options_api branch from 24a210d to 9fb1dfc Compare August 8, 2017 14:44

jntullo suggested changes Aug 8, 2017

View reviewed changes

enoodle force-pushed the provider_options_api branch from 9fb1dfc to c256d49 Compare August 8, 2017 15:10

abellotti added the enhancement label Aug 8, 2017

jntullo reviewed Aug 8, 2017

View reviewed changes

enoodle force-pushed the provider_options_api branch from c256d49 to cfc7b9a Compare August 8, 2017 16:02

imtayadeway reviewed Aug 8, 2017

View reviewed changes

enoodle force-pushed the provider_options_api branch 2 times, most recently from fe1ba1f to af45586 Compare August 9, 2017 08:58

enoodle closed this Aug 9, 2017

enoodle reopened this Aug 9, 2017

enoodle force-pushed the provider_options_api branch from af45586 to 1e2b12f Compare August 9, 2017 09:02

imtayadeway suggested changes Aug 9, 2017

View reviewed changes

imtayadeway reviewed Aug 9, 2017

View reviewed changes

enoodle force-pushed the provider_options_api branch 2 times, most recently from 317ad45 to 7f6d4c0 Compare August 9, 2017 18:09

imtayadeway reviewed Aug 9, 2017

View reviewed changes

enoodle force-pushed the provider_options_api branch from 7f6d4c0 to accf865 Compare August 9, 2017 20:31

chessbyte assigned abellotti Aug 9, 2017

enoodle force-pushed the provider_options_api branch 2 times, most recently from c5a6fe3 to 3150fdb Compare August 10, 2017 13:19

enoodle mentioned this pull request Aug 10, 2017

Add the ability to edit options settings for providers ManageIQ/manageiq-ui-classic#1652

Merged

imtayadeway reviewed Aug 10, 2017

View reviewed changes

enoodle force-pushed the provider_options_api branch from 3150fdb to 44e7509 Compare August 13, 2017 12:06

enoodle mentioned this pull request Aug 13, 2017

Adding options_description class method to base_manager ManageIQ/manageiq#15799

Merged

imtayadeway approved these changes Aug 14, 2017

View reviewed changes

enoodle changed the title ~~OPTIONS for /api/providers~~ [WIP] OPTIONS for /api/providers Aug 15, 2017

miq-bot added the wip label Aug 15, 2017

enoodle changed the title ~~[WIP] OPTIONS for /api/providers~~ OPTIONS for /api/providers Aug 24, 2017

enoodle closed this Aug 24, 2017

enoodle reopened this Aug 24, 2017

miq-bot removed the wip label Aug 24, 2017

imtayadeway reviewed Aug 24, 2017

View reviewed changes

enoodle force-pushed the provider_options_api branch from 1aa3e25 to 87c533f Compare August 24, 2017 14:41

enoodle force-pushed the provider_options_api branch from 87c533f to 37f3b81 Compare September 3, 2017 13:07

use OPTIONS /api/providers

cb3b52c

enoodle force-pushed the provider_options_api branch from 37f3b81 to cb3b52c Compare September 4, 2017 08:40

enoodle closed this Sep 4, 2017

enoodle mentioned this pull request Sep 4, 2017

use OPTIONS /api/providers #43

Merged

chrisarcand mentioned this pull request Nov 2, 2017

Filters vs query params for endpoints #108

Closed

OPTIONS for /api/providers #3

OPTIONS for /api/providers #3

Conversation

enoodle commented Aug 8, 2017

enoodle commented Aug 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imtayadeway commented Aug 9, 2017

enoodle commented Aug 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enoodle Aug 13, 2017 • edited Loading

Choose a reason for hiding this comment

enoodle commented Aug 14, 2017

imtayadeway left a comment

Choose a reason for hiding this comment

enoodle commented Aug 15, 2017

enoodle commented Aug 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abellotti commented Aug 24, 2017

enoodle commented Aug 24, 2017

abellotti commented Aug 24, 2017 • edited Loading

enoodle commented Aug 25, 2017

abellotti commented Aug 30, 2017

enoodle commented Sep 3, 2017

miq-bot commented Sep 4, 2017

enoodle commented Sep 4, 2017

enoodle Aug 13, 2017 •

edited

Loading

abellotti commented Aug 24, 2017 •

edited

Loading