Regex event detection

[kbrock]

Part of #17201

Overview

The serialization of regular expressions in our yaml file is giving us issues when transitioning the data over json.
this breaks the react code that is transferring it

See issues in ManageIQ/manageiq-api#979

Solution

Store strings in yaml that are strings, but are detectable as being a regular expression.
This does transition the task of detecting it is a regular expression from yaml to ruby, but since that code already has special provisions for regular expression, it didn't seem to add too much additional knowledge.

Implementation

This is a single line change. but I had to do a little too much to get there.

Please look at individual commits to see what is the refactor and what is the actual change.

Tangent

I did not feel comfortable adding the regular expression logic in the loop. It is probably the same thing but this loop triggers events and needs to be as tight as possible.

It seems like we may be able to avoid this conversion every time. Especially since this is mostly relatively static. If we did that, we would see a big performance boost

[kbrock]

sigh. stupid works for me locally test.
rubocops look like they are comparing old code even though git status shows that the branch is up to date.
pushing formatting changes and seeing if that fixes failed test.

[Fryguy]

This will require changing the defaults in each of the providers. This feature was added specifically to allow providers to provide more sane defaults than listing out every single possible event (e.g. https://github.com/ManageIQ/manageiq-providers-redfish/blob/master/config/settings.yml#L8-L9)

We may also need a data migration if customers could change these. @agrare ?

[Fryguy]

@agrare Please also review.

[Fryguy]

LGTM when mixed with the provider PRs.

Can you update the OP to put all the links to the provider PRs, and if you haven't already, can we run a cross-repo-test?

[Fryguy]

Actually, I'm looking closer and I think there's a bug -

I don't see any tests that actually test that the regex is being applied. The test from the regex just asks if it's in the list of filters to apply, but not if the filtering is actually being applied.

I have a feeling based on the code, that it's not actually working, because I don't see the code stripping off the leading and trailing / characters.

[1] pry(main)> "abc".match("b.")
=> #<MatchData "bc">
[1] pry(main)> "abc".match("/b./")
=> nil

[kbrock]

@miq-bot cross_repo_test ManageIQ/manageiq-providers-azure_stack#54 ManageIQ/manageiq-providers-redfish#133 ManageIQ/manageiq-providers-nuage#237 includes #20907

[miq-bot]

Checked commit kbrock@cdf4c28 with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. ⭐

[Fryguy]

The test from the regex just asks if it's in the list of filters to apply, but not if the filtering is actually being applied.

manageiq/spec/models/ems_event_spec.rb

Lines 416 to 420 in c52be90

	it 'returns the provider event if configured' do
	expect(described_class.event_groups[:addition][:critical]).to include('CloneTaskEvent')
	expect(described_class.event_groups[:addition][:critical]).to include(provider_event)
	expect(described_class.event_groups[:addition][:warning]).to include(provider_regex)
	end

@kbrock Can you verify if there are tests around this functionality, and if not can you add a couple?

[agrare]

👍 looks great, much more targeted than the previous version

Overall LGTM...I still think it needs tests that exercise the actual regex.

[agrare]

@miq-bot cross-repo-tests #20907, ManageIQ/manageiq-providers-nuage#237, ManageIQ/manageiq-providers-azure_stack#54, ManageIQ/manageiq-providers-redfish#133

[kbrock]

The test from the regex just asks if it's in the list of filters to apply, but not if the filtering is actually being applied.

manageiq/spec/models/ems_event_spec.rb

Lines 416 to 420 in c52be90

	it 'returns the provider event if configured' do
	expect(described_class.event_groups[:addition][:critical]).to include('CloneTaskEvent')
	expect(described_class.event_groups[:addition][:critical]).to include(provider_event)
	expect(described_class.event_groups[:addition][:warning]).to include(provider_regex)
	end

@kbrock Can you verify if there are tests around this functionality, and if not can you add a couple?
-- @Fryguy

Not sure if I already answered this question out of band. We do have a test that ensures regular expressions are applied and in the right order. I feel that the regular expression behavior has not changed (should not change) so the existing test will do what we need.

manageiq/spec/models/ems_event_spec.rb

Lines 445 to 450 in c52be90

	it 'returns the group, level and group name of a regex-matched event' do
	group, level = described_class.group_and_level('SomeMatchingEvent')
	expect(group).to eq(:addition)
	expect(level).to eq(:warning)
	expect(described_class.group_name(group)).to eq('Creation/Addition')
	end

[agrare]

@kbrock I want to confirm the merge order here, first this core pr, then the provider prs, then schema?

[kbrock]

@agrare yea - I'd do nuage, core, then other providers.

You'd need to kick nuage again to ensure that it is green, but this order ensures the most green/happy builds
The schema can happen a the end with no rush.

if you prefer, we can change the code back to having core accept both inputs - so it would be green. Then we could remove at a later time when everything is green. (you and fry asked me to take that out. just offering if you have changed your mind)

[agrare]

Nuage is green here https://travis-ci.com/github/ManageIQ/manageiq-cross_repo-tests/builds/221501032 so I'm good with that