Blacklist Config Values

[jntullo]

Proposed solution to resolve #134

Rather than rescuing an error, blacklist the :system and :display values. Not allowing the :display value to be called will resolve issues with the config being displayed in the console output, and removing the call to :system removes the need for the rescue block.

cc: @himdel @chrisarcand

@miq-bot add_label bug
@miq-bot assign @abellotti

[himdel]

@jntullo I don't think this is a good solution.

As mentioned in the issue, this is not about 2 methods. This is about every method defined on Object, OpenStruct or Config::Options. Right now, we're hitting problems with 2 of them but that's not really future-proof.

(I'm almost sure if we ever add a gem that extends Object to add a method named after anything already in the config, it will break the API equally well.)

Can we use a whitelist instead?

(But IMO the only fix that would really fix the issue is replace Config::Options with something that doesn't do send.)

[chrisarcand]

I don't think this is a good solution.

I think this is a great solution - when replacing the config gem quickly or easily is certainly not an option.

[himdel]

@chrisarcand I don't know how much it's being used and what all is used from it, but.. would it really be that hard to replace?

But.. assuming the gem can't be replaced, I'm good with this. I still think we'll hit these problems again, but .. eh, maybe not soon.

[Fryguy]

I agree with @himdel as this is not API's concern to blacklist the config gem. If anything, it belongs in Vmdb::Settings. See my comment in the issue : #134 (comment)

[imtayadeway]

As mentioned in the issue, this is not about 2 methods. This is about every method defined on Object, OpenStruct or Config::Options. Right now, we're hitting problems with 2 of them but that's not really future-proof.

I think a better solution to this is: don't do that. Having a blacklist elsewhere to ensure we don't add conflicting keys to the config I'm all for. The problem in this case was that we were looking for settings that weren't there, and that's the part I'm suggesting we tackle.

[Fryguy]

The problem in this case was that we were looking for settings that weren't there, and that's the part I'm suggesting we tackle.

I don't disagree but this is not API's concern...it's a general concern, and can happen to anyone that tries to access the config. Even so (and I commented in the other thread with this), if all that's trying to be done is verify the presence of a key, then you can also use Settings.to_h.include?(key), which avoids the "doing a key lookup by actually calling a method", which in general you shouldn't do with Settings anyway since any random key is available on an OpenStruct.

Settings.sdkfjhsdkfjh
# => nil
Settings.display
# OMGSOMUCHOUTPUT=>nil

Settings.to_h.include?(:sdkfjhsdkfjh)
# => false
Settings.to_h.include?(:display)
# => false

# And for fun ;)
Settings.sdkfjhsdkfjh = 123
Settings.to_h.include?(:sdkfjhsdkfjh)
=> true

[miq-bot]

Checked commits jntullo/manageiq-api@fa8e1ef~...00d91a7 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

[abellotti]

Thanks @jntullo for fixing this. 👍

[cben]

Neat! Could you rename the PR (and maybe edit description) to reflect the final solution?