Fix brakeman fingerprint patch on clones

[Fryguy]

@jrafanie Please review.

[Fryguy]

@jrafanie When I ran these tests without the code fix, all of them passed except this one, and within this one the engine one passed but the core case did not. So, the code fix specifically fixes this case without breaking the other cases.

[miq-bot]

Checked commits Fryguy/manageiq@b4ad486~...00b8966 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
2 files checked, 7 offenses detected

lib/extensions/brakeman_fingerprint_patch.rb

• ❗ - Line 43, Col 73 - Style/RedundantSelf - Redundant self detected.
• ❗ - Line 44, Col 7 - Style/RedundantSelf - Redundant self detected.
• ❗ - Line 68, Col 22 - Style/RedundantSelf - Redundant self detected.
• ❗ - Line 69, Col 22 - Style/RedundantSelf - Redundant self detected.

spec/lib/extensions/brakeman_fingerprint_patch_spec.rb

• 💣 💥 🔥 🚒 - Line 6, Col 22 - Lint/Syntax - unexpected token tEQL
  (Using Ruby 2.7 parser; configure using TargetRubyVersion parameter, under AllCops)
• 💣 💥 🔥 🚒 - Line 7, Col 22 - Lint/Syntax - unexpected token tEQL
  (Using Ruby 2.7 parser; configure using TargetRubyVersion parameter, under AllCops)
• 💣 💥 🔥 🚒 - Line 8, Col 20 - Lint/Syntax - unexpected token tEQL
  (Using Ruby 2.7 parser; configure using TargetRubyVersion parameter, under AllCops)

[Fryguy]

So, when have a core cloned underneath the engine we have 2 cases

• issue in an engine file, such as: /Users/user/dev/manageiq-ui-classic/app/controllers/application_controller.rb
• issue in a core file, such as /Users/user/dev/manageiq-ui-classic/spec/manageiq/lib/ansible/runner.rb

In both of those cases, we get an engine_path, but in reality, only the issue in an engine file one is actually in the engine, whereas the core file just happens to be under the engine directory, but is actually an issue in core. So, we do additional checks to

• see if the Rails root starts with the engine_path, which tells us we are in the core cloned underneath the engine case
• then see if the issue path starts with the rails root, which tells us that the file is actually a core issue and not an engine issue

[jrafanie]

Wow, that's too bad we need to check it like this in a patch of brakeman.

Just checking... Do we still think it's worth checking all of the engines here? I know it's been a bit frustrating that an engine issue was causing unrelated core test failures. Is it just more convenient to pull in all the engines and test each one?

Honestly, if we tested in UI classic only, the rest of the engines could probably be included in the core tests. UI classic is likely the noisest offender for brakeman issues outside of core. I'm just wondering if it's worth it.

[Fryguy]

It's a really good idea. I think what I'd like to do is get this merged to get us green then I can investigate that

[Fryguy]

Ok, I researched this a little, and I thikn the way we have it is the best way. The brakeman app technically always runs from the main app, and what we do is just tell it more engines to check. From core, we just loop through all of the engines and add all of them. From an engine, we just add the engine itself only.

[jrafanie]

Right, I think the problem is where the problem is reported. If we add something in an engine that breaks core, it makes core red and we become hesitant to merge while it's being fixed whereas the engine is not aware of the problem. We can look at it as a followup, I just wanted to make sure the complexity still made sense.

[Fryguy]

@jrafanie Regarding the rubocops

• The redundant self's are in the original code, and for my patch I continued that pattern so it was consistent.
• The is giving syntax errors because it's checking against Ruby 2.7 syntax, however that code is valid ruby 30 and 3.1 syntax. If I convert it to single-line method like def location; {}; end, then we'll get a rubocop error on not using the newer single-line method syntax. I just need to update the bot/settings to check ruby 3.0 syntax.