Add Alert Definition Profiles (MiqAlertSet) REST API support #14438

dkorn · 2017-03-21T19:21:17Z

Adding CRUD actions support to alert definition profile API endpoint

dkorn · 2017-03-21T19:21:24Z

@miq-bot add_label enhancement, api, control

dkorn · 2017-03-21T19:33:09Z

@moolitayer @zeari @simon3z @abellotti please review

simon3z · 2017-03-22T10:49:22Z

@moolitayer I think you should review this one given that it is directly related to your work on alerts (and what we need for dwh).

moolitayer · 2017-03-22T18:13:22Z

app/controllers/api/alert_definition_profiles_controller.rb

+    REQUIRED_FIELDS = %w(description mode).freeze
+
+    def create_resource(type, id, data = {})
+      assert_id_not_specified(data, type)


Safe to remove this as it is called in api::base_controller:generic (I checked :beer:)

done, thanks!

moolitayer · 2017-03-22T18:22:40Z

app/controllers/api/subcollections/alert_definitions.rb

+    module AlertDefinitions
+      def alert_definitions_query_resource(object)
+        return {} unless object.respond_to?(:miq_alerts)
+        object.miq_alerts


That would give us a MiqPolicySet's MiqPolicies but will not give us a VM MiqPolicies.
Is that intentional?

If you need that you should call get_policies (see miq_policy_mixin.rb)

you mean MiqAlertSet and MiqAlert right?

Are you sure about it? seemed to me it returns the object's miq_alerts, regarding of the set type.

BTW, to the best of my knowledge, there is no miq_alert_mixin.rb to offer get_alerts

Sorry I was confused with policies (I keep thinking of the two together). This looks good, we only need to get the miq alerts belonging to a miq alert profile

moolitayer · 2017-03-22T18:27:44Z

config/api.yml

+        :identifier: alert_definition_profile_delete
+      :delete:
+      - :name: delete
+        :identifier: alert_definition_profile_delete


we need two kinds of assign here:
Assign to object and assign a policy to this profile.
(we cannot assign to object through an object subcollection since the target entities we need have no rest endpoints for example container_node.rb)

@abellotti me and @dkorn were discussing this and weren't sure how to do it.

Let's wait for @abellotti 's suggestion :)

are we talking about assigning an MiqPolicy to a CI as well as an alert_definition_profile ?

Yes. assigning miq alert to miq alert profile seems straight forward. Assigning miq alert profile to object is more complex. You can assign a miq alert profile to a specific object, all object of a provider or all object in the system ("the Enterprise"). And the UI currently has the information of which alerts can be assigned to what (based on the target type, some can be assign only to one object others only to all the objects in the system)

I don't think this would be easy to get at one shot but what we need most urgent is to assign MA to MAP and MAP to all container nodes in the system. Of course the design should allow growing other examples next

Adding CRUD actions support to alert definition profile API endpoint

dkorn · 2017-03-26T09:54:05Z

@abellotti, @moolitayer and I tried to find an implementation for assigning policies to a policy profile (to take example from) but couldn't find it.
Can you confirm it is indeed not supported?

abellotti · 2017-03-27T14:56:43Z

MiqPolicySet does not have the add_policy method exposed to it, so I'd say not supported today. Also, could't find functionality in the UI.

dkorn · 2017-03-28T11:24:06Z

@abellotti If you go to Control -> Explorer -> Policy Profiles you can add policies to an existing policy profile or create a new one and select policies to it (@moolitayer keep me honest here).

@abellotti Is there a reason not to expose adding MiqAlert (alert definitions) to MiqAlertSet (alert definition profiles)?

abellotti · 2017-03-28T13:58:55Z

@dkorn I think you're right, in my DB I only had a read-only policy profile.

looks like I can attempt POST /api/policy_profiles/:id/policies in the API, with the assign and unassign actions like the other CI's, however I'm getting undefine method 'add_policy' and 'remove_policy'. So it would be good to know what methods the UI uses for this, and why we can't expose add_policy and remove_policy so it will work like like the other CI's. Also, some of the policy_profiles cannot be edited so we need to support the same and only expose the actions when in effect (driven by validate_action options in api.yml).

dkorn · 2017-03-30T11:31:48Z

@abellotti the UI works with the MiqSet add_member, remove_member actions.

I submitted a PR to expose these actions in case the object to assign the policies to is a policy profile: #14575
Hope this approach is acceptable.

dkorn · 2017-04-04T12:26:32Z

@abellotti can we progress with this PR (alert profiles CRUD) without the assigning of alerts to alerts profiles? (which is also discussed regarding policies in another PR of mine #14575)

dkorn · 2017-04-19T09:40:06Z

@abellotti bumping this one up

Loicavenel · 2017-04-20T12:09:02Z

@abellotti Hi Alberto, we need your help on this one..

abellotti

Hi @dkorn, this looks good, a couple of minor changes. Thanks !!

/cc @Loicavenel

abellotti · 2017-04-20T17:19:29Z

app/controllers/api/subcollections/alert_definitions.rb

+    module AlertDefinitions
+      def alert_definitions_query_resource(object)
+        return {} unless object.respond_to?(:miq_alerts)
+        object.miq_alerts


probably should return [], above 2 lines can just be:

object.respond_to?(:miq_alerts) ? object.miq_alerts : []

Done, thanks!

abellotti · 2017-04-20T17:22:38Z

spec/requests/api/alert_definitions_spec.rb

@@ -143,3 +144,115 @@
    expect(alert_definitions.second.reload.description).to eq("Updated Test Alert 2")
  end
 end
+
+describe "Alerts Definition Profiles API" do
+  it "reads 2 alert definition profiles as a collection" do


let's add a test for accessing without appropriate role and expect the forbidden error. Thanks.

Added, thanks!

abellotti · 2017-04-20T17:24:03Z

spec/requests/api/alert_definitions_spec.rb

+    api_basic_authorize action_identifier(:alert_definition_profiles, :delete, :resource_actions, :post)
+    alert_definition_profile = FactoryGirl.create(:miq_alert_set)
+    run_post(alert_definition_profiles_url(alert_definition_profile.id), gen_request(:delete))
+    expect(response).to have_http_status(:ok)


just esthetics/grouping, if you can add a blank line between the run_* commands and the expects, here and the tests below. Thanks.

Done, thanks!

abellotti

this is looking great, minor update to api.yml and we're good. Thanks!!

abellotti · 2017-04-26T17:34:17Z

config/api.yml

+    :identifier: alert_definition_profile
+    :options:
+    - :collection
+    - :subcollection


I didn't see alert_definition_profiles declared as a subcollection of others, so probably drop line 96.

Done, thanks!

* added tests for accessing without appropriate role * small refactor to the subcollection controller * esthetic changes in specs

miq-bot · 2017-04-27T09:02:29Z

Checked commits dkorn/manageiq@4067acb~...f509b39 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks fine. 🍰

abellotti · 2017-04-27T11:04:26Z

Thanks @dkorn for the update, this looks great!! 👍

miq-bot added api core/policy enhancement labels Mar 21, 2017

chessbyte assigned abellotti Mar 21, 2017

dkorn force-pushed the alert_definition_profiles branch from b367c9a to 20a08c5 Compare March 22, 2017 07:54

moolitayer reviewed Mar 22, 2017

View reviewed changes

Add Alert Definition Profiles (MiqAlertSet) REST API support

4067acb

Adding CRUD actions support to alert definition profile API endpoint

dkorn force-pushed the alert_definition_profiles branch from 20a08c5 to 4067acb Compare March 23, 2017 13:18

abellotti reviewed Apr 20, 2017

View reviewed changes

dkorn closed this Apr 25, 2017

dkorn reopened this Apr 25, 2017

abellotti reviewed Apr 26, 2017

View reviewed changes

Review revisions

f509b39

* added tests for accessing without appropriate role * small refactor to the subcollection controller * esthetic changes in specs

dkorn force-pushed the alert_definition_profiles branch from 9c76216 to f509b39 Compare April 27, 2017 09:00

abellotti added this to the Sprint 60 Ending May 8, 2017 milestone Apr 27, 2017

abellotti merged commit 56040f2 into ManageIQ:master Apr 27, 2017

dkorn deleted the alert_definition_profiles branch April 27, 2017 11:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Alert Definition Profiles (MiqAlertSet) REST API support #14438

Add Alert Definition Profiles (MiqAlertSet) REST API support #14438

dkorn commented Mar 21, 2017

dkorn commented Mar 21, 2017

dkorn commented Mar 21, 2017

simon3z commented Mar 22, 2017

moolitayer Mar 22, 2017

dkorn Mar 23, 2017

moolitayer Mar 22, 2017

dkorn Mar 23, 2017

moolitayer Mar 23, 2017

moolitayer Mar 22, 2017

dkorn Mar 23, 2017

abellotti Mar 23, 2017

moolitayer Mar 23, 2017

dkorn commented Mar 26, 2017

abellotti commented Mar 27, 2017

dkorn commented Mar 28, 2017 •

edited

Loading

abellotti commented Mar 28, 2017

dkorn commented Mar 30, 2017

dkorn commented Apr 4, 2017

dkorn commented Apr 19, 2017

Loicavenel commented Apr 20, 2017

abellotti left a comment

abellotti Apr 20, 2017

dkorn Apr 24, 2017

abellotti Apr 20, 2017

dkorn Apr 24, 2017

abellotti Apr 20, 2017

dkorn Apr 24, 2017

abellotti left a comment

abellotti Apr 26, 2017

dkorn Apr 27, 2017

miq-bot commented Apr 27, 2017

abellotti commented Apr 27, 2017

Add Alert Definition Profiles (MiqAlertSet) REST API support #14438

Add Alert Definition Profiles (MiqAlertSet) REST API support #14438

Conversation

dkorn commented Mar 21, 2017

dkorn commented Mar 21, 2017

dkorn commented Mar 21, 2017

simon3z commented Mar 22, 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

dkorn commented Mar 26, 2017

abellotti commented Mar 27, 2017

dkorn commented Mar 28, 2017 • edited Loading

abellotti commented Mar 28, 2017

dkorn commented Mar 30, 2017

dkorn commented Apr 4, 2017

dkorn commented Apr 19, 2017

Loicavenel commented Apr 20, 2017

abellotti left a 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

abellotti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Apr 27, 2017

abellotti commented Apr 27, 2017

dkorn commented Mar 28, 2017 •

edited

Loading