Move event filters into the database. #3636

lfu · 2015-07-28T21:49:20Z

Changes for phase 1 in #3273.

Fryguy · 2015-07-28T21:55:21Z

app/models/event_blacklist.rb

+  default_value_for :enabled, true
+  after_save        :queue_sync_filtered_events
+  after_destroy     :queue_sync_filtered_events
+  after_destroy { |r| _log.info("Event filter [#{r.event_name}] for provider #{r.provider_model} with ID [#{r.ems_id}] has been deleted by user [#{self.class.current_userid}]") }


Do we need this? If so, perhaps it should go into the AuditLog instead of evm log?

Fryguy · 2015-07-28T22:02:40Z

@lfu Looking good!

gmcculloug · 2015-07-29T21:30:03Z

app/models/event_blacklisting.rb

+  after_destroy     :queue_sync_filtered_events
+  after_destroy { |r| $audit_log.success("Event filter [#{r.event_name}] for provider #{r.provider_model} with ID [#{r.ems_id}] has been deleted by user [#{self.class.current_userid}]") }
+
+  YAML_FILE ||= Rails.root.join("db", "fixtures", "#{self.to_s.pluralize.underscore}.yml")


You do not need 'self' here and the to_s should be name: name.pluralize.underscore

chessbyte · 2015-07-30T15:04:54Z

app/models/event_blacklisting.rb

@@ -0,0 +1,76 @@
+class EventBlacklisting < ActiveRecord::Base


There was probably a discussion I missed on the naming of EventBlacklisting. Looking up the meaning of a listing shows a printed list as the main definition. To avoid ambiguity I would change this name to EventBlacklistEntry

#3636 (diff)

gmcculloug · 2015-07-30T17:55:03Z

@Fryguy @matthewd Sounds like it would be better to move the default filters into each provider model as an array constant (instead of a single file with all providers) and for seeding we could process the descendants of the ExtManagementSystem model and process any that define the blacklist constant.

Something like:

  def self.seed
    MiqRegion.my_region.lock do
      ExtManagementSystem.descendants.each do |ems| 
        if ems.const_defined?(:DEFAULT_BLACKLISTED_EVENTS, false)
          ...

The seeding code will create any records that do not exist. By using the enabled flag a user can decide if they want to process one of the default events without removing it from the list.

Thoughts?

matthewd · 2015-07-30T18:09:39Z

You probably want a , false in there, but otherwise 👍

gmcculloug · 2015-07-30T18:15:56Z

Thanks, I updated the code snippet with that.

Fryguy · 2015-07-30T18:43:36Z

Yeah, I like that idea. Alternately it could be a class method. This would give the provider plugin author the flexibility to use a constant or a yaml file or whatever they want. Thoughts?

matthewd · 2015-07-30T18:48:36Z

I lean toward the constant, because it "feels" more right: if you make your method clever enough that its value might change at some point, you're likely in for a surprise.

If the author happens to be particularly keen on YAML, they can just read it while setting the constant 😄

chessbyte · 2015-07-30T19:14:30Z

I lean more toward a method that can be subclassed, overridden and memo-ized at will.

To me, if ems.const_defined?(:DEFAULT_BLACKLISTED_EVENTS, false) is a code smell. It should just read ems.blacklisted_events and the ExtManagementSystem class will have a base method that returns an empty array.

matthewd · 2015-07-30T19:31:09Z

¯_(ツ)_/¯ that level of dynamism seems like overkill for something we were about to put in a static text file, but as long as each provider owns its part of the list in some way, I'm happy.

I was assuming we specifically wouldn't want it to be inherited by children automatically... otherwise it could skip the if even with the constant.

chessbyte · 2015-07-30T19:41:23Z

@matthewd I think I am reacting to whether DEFAULT_BLACKLISTED_EVENTS is defined per EMS/Provider. Perhaps the constant should always be defined and set to [] to signify that there are no default blacklisted events.

matthewd · 2015-07-30T20:34:32Z

Yeah, I think my imagined scenario where a class halfway up the inheritance hierarchy might define values, and that we would not want to automatically inflict those upon its subclasses, is actually pretty unlikely. So, either a method or a constant, defined as [] on ExtManagementSystem and inherited/overridden by subclasses, sounds good to me. I like the constant for the "set this once, at boot; no second chances" taste... but then, I can't really speak to what precedent we have for similar notions elsewhere.

However, it now occurs to me that blacklist + disabled is an unfortunate / possibly confusing double-negative. In order to receive the X event, I must ensure its blacklisting is disabled; if I don't want the event, it must be enabled. No exciting new names are leaping to mind, though, so perhaps that's just a problem for another day.

gmcculloug · 2015-07-30T20:53:15Z

We started off calling the items filters and the filter could be enabled/disabled. Does it make sense to keep the word "filter" in the name: EventBlacklistFilter?

matthewd · 2015-07-30T20:55:28Z

EventFilter + blacklist?

chessbyte · 2015-07-30T21:11:55Z

@matthewd can you elaborate on your suggestion of EventFilter + blacklist?

gmcculloug · 2015-08-03T19:59:39Z

Marking as WIP while we work on a migration to handle user added filters.

miq-bot · 2015-08-04T20:53:13Z

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

Fix ManageIQ#3273

… method per provider. Fix ManageIQ#3273

…pendent => destroy Fix # 3273

Fix ManageIQ#3273

Fix # 3273

…ation into BlacklistedEvents. Only user added events will be migrated. The system events will be added during seeding. Fix ManageIQ#3273.

miq-bot · 2015-08-12T19:29:50Z

Checked commits lfu@c32d39d .. lfu@de7aa4e with rubocop 0.32.1 and haml-lint 0.13.0
14 files checked, 22 offenses detected

app/models/blacklisted_event.rb

🔶 - Line 10, Col 5 - Style/GlobalVars - Do not introduce global variables.
🔶 - Line 14, Col 5 - Style/GlobalVars - Do not introduce global variables.
🔶 - Line 21, Col 5 - Style/GlobalVars - Do not introduce global variables.
🔹 - Line 10, Col 121 - Metrics/LineLength - Line is too long. [165/120]
🔹 - Line 14, Col 121 - Metrics/LineLength - Line is too long. [157/120]
🔹 - Line 21, Col 121 - Metrics/LineLength - Line is too long. [180/120]
🔹 - Line 27, Col 121 - Metrics/LineLength - Line is too long. [132/120]
🔹 - Line 41, Col 121 - Metrics/LineLength - Line is too long. [128/120]

db/migrate/20150806194147_migrate_filtered_events_to_blacklisted_events.rb

🔹 - Line 11, Col 3 - Metrics/AbcSize - Assignment Branch Condition size for up is too high. [20.32/15]
🔹 - Line 40, Col 3 - Metrics/MethodLength - Method has too many lines. [29/25]

spec/migrations/20150806194147_migrate_filtered_events_to_blacklisted_events_spec.rb

spec/models/blacklisted_event_spec.rb

🔹 - Line 5, Col 121 - Metrics/LineLength - Line is too long. [128/120]

gmcculloug · 2015-08-14T21:01:49Z

👍

Move event filters into the database.

matthewd · 2015-08-22T01:01:06Z

db/migrate/20150804194147_create_blacklisted_events.rb

+      t.bigint  :ems_id
+      t.boolean :system
+      t.boolean :enabled
+      t.timestamps


This has added a bunch of noise to the migration specs...

DEPRECATION WARNING: #timestamps was called without specifying an option for null. In Rails 5, this behavior will change to null: false. You should manually specify null: true to prevent the behavior of your existing migrations from changing. (called from block in change at db/migrate/20150804194147_create_blacklisted_events.rb:9)

Fryguy reviewed Jul 28, 2015
View reviewed changes

Fryguy added enhancement core/events labels Jul 28, 2015

lfu force-pushed the filtered_provider_events_3273 branch from 9273cfb to b57c7f1 Compare July 29, 2015 18:19

gmcculloug reviewed Jul 29, 2015
View reviewed changes

lfu force-pushed the filtered_provider_events_3273 branch from b57c7f1 to 8152bea Compare July 29, 2015 21:49

chessbyte reviewed Jul 30, 2015
View reviewed changes

lfu force-pushed the filtered_provider_events_3273 branch from 2126640 to 5e6b90f Compare August 3, 2015 16:23

gmcculloug changed the title ~~Move event filters into the database.~~ [WIP] Move event filters into the database. Aug 3, 2015

gmcculloug added the wip label Aug 3, 2015

lfu force-pushed the filtered_provider_events_3273 branch 5 times, most recently from e80b0e3 to 2ddd6ed Compare August 10, 2015 19:03

lfu added 8 commits August 12, 2015 14:54

Add migration for BlacklistedEvent.

c32d39d

Fix ManageIQ#3273

Add class BlacklistedEvent.

fa6c283

Fix ManageIQ#3273

Move filtered_events section from event_handling.tmp.yml into a class…

6a2affe

… method per provider. Fix ManageIQ#3273

Add has_many relationship from provider to blacklisted_events with de…

a9dbad2

…pendent => destroy Fix # 3273

Remove existing filter logic from ems_event.rb.

de53bf3

Fix ManageIQ#3273

Add spec for BlacklistedEvent.

af144d9

Fix # 3273

Changes to sync the blacklisted events.

f38e1f9

Add migration to move existing user added filtered_events in Configur…

de7aa4e

…ation into BlacklistedEvents. Only user added events will be migrated. The system events will be added during seeding. Fix ManageIQ#3273.

lfu force-pushed the filtered_provider_events_3273 branch from 2ddd6ed to de7aa4e Compare August 12, 2015 18:55

gmcculloug removed the wip label Aug 13, 2015

gmcculloug changed the title ~~[WIP] Move event filters into the database.~~ Move event filters into the database. Aug 13, 2015

gmcculloug added a commit that referenced this pull request Aug 14, 2015

Merge pull request #3636 from lfu/filtered_provider_events_3273

645dc9d

Move event filters into the database.

gmcculloug merged commit 645dc9d into ManageIQ:master Aug 14, 2015

gmcculloug added this to the Sprint 28 Ending August 24, 2015 milestone Aug 14, 2015

lfu deleted the filtered_provider_events_3273 branch August 19, 2015 18:54

matthewd reviewed Aug 22, 2015
View reviewed changes

blomquisg mentioned this pull request Apr 5, 2016

MW Hawkular provider initial event catcher #7395

Merged

lfu mentioned this pull request Mar 20, 2017

Add openstack infra provider event blacklist #14369

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move event filters into the database. #3636

Move event filters into the database. #3636

lfu commented Jul 28, 2015

Fryguy Jul 28, 2015

Fryguy commented Jul 28, 2015

gmcculloug Jul 29, 2015

chessbyte Jul 30, 2015

Fryguy Jul 30, 2015

gmcculloug commented Jul 30, 2015

matthewd commented Jul 30, 2015

gmcculloug commented Jul 30, 2015

Fryguy commented Jul 30, 2015

matthewd commented Jul 30, 2015

chessbyte commented Jul 30, 2015

matthewd commented Jul 30, 2015

chessbyte commented Jul 30, 2015

matthewd commented Jul 30, 2015

gmcculloug commented Jul 30, 2015

matthewd commented Jul 30, 2015

chessbyte commented Jul 30, 2015

gmcculloug commented Aug 3, 2015

miq-bot commented Aug 4, 2015

miq-bot commented Aug 12, 2015

gmcculloug commented Aug 14, 2015

matthewd Aug 22, 2015

		@@ -0,0 +1,76 @@
		class EventBlacklisting < ActiveRecord::Base

Move event filters into the database. #3636

Move event filters into the database. #3636

Conversation

lfu commented Jul 28, 2015

Fryguy Jul 28, 2015

Choose a reason for hiding this comment

Fryguy commented Jul 28, 2015

gmcculloug Jul 29, 2015

Choose a reason for hiding this comment

chessbyte Jul 30, 2015

Choose a reason for hiding this comment

Fryguy Jul 30, 2015

Choose a reason for hiding this comment

gmcculloug commented Jul 30, 2015

matthewd commented Jul 30, 2015

gmcculloug commented Jul 30, 2015

Fryguy commented Jul 30, 2015

matthewd commented Jul 30, 2015

chessbyte commented Jul 30, 2015

matthewd commented Jul 30, 2015

chessbyte commented Jul 30, 2015

matthewd commented Jul 30, 2015

gmcculloug commented Jul 30, 2015

matthewd commented Jul 30, 2015

chessbyte commented Jul 30, 2015

gmcculloug commented Aug 3, 2015

miq-bot commented Aug 4, 2015

miq-bot commented Aug 12, 2015

gmcculloug commented Aug 14, 2015

matthewd Aug 22, 2015

Choose a reason for hiding this comment