Prioritize SubscriptionManager over Rhn, and other warning fixes #200

NickLaMuro · 2018-03-13T23:46:55Z

Now the LinuxAdmin::Rhn has been deprecated in #198 , this means that every instantiation of LinuxAdmin::Rhn will then add a deprecation warning to STDOUT.

Previously, this would also be triggered even if you used the public class methods on LinuxAdmin::SubscriptionManager (the deprecation's suggested replacement), because under the hood it determine which to use (Rhn or SubscriptionManager) by initializing each and calling their registered? methods.

Furthermore, these means if Rhn is registered on the system in addition to the preferred SubscriptionManager, it the LinuxAdmin::SubscriptionManager.validate_credentials class method would actually end up calling LinuxAdmin::Rhn.validate_credentials instead because of the preference.

The fix

It changes the priority to favor SubscriptionManager over Rhn
Adds a new initial check that will check against the subclasss first before trying the other subclasses in the order of our choosing.
Fixes some deprecation warnings in specs
- Removes printing the "[DEPRECATION] 'LinuxAdmin::Rhn'..." in the RegistrationSystem specs (skipped the other case of it for now)
- Removes ruby warning for trying to access @registration_type without it being defined (can cause warnings in consumers of this gem)

Links

Related effort: Fix LinuxAdmin::Rhn deprecation warning... kinda manageiq#17153

Now the `LinuxAdmin::Rhn` has been deprecated: ManageIQ#198 1ifd7c4f788ce34200ff09ef56176397306904f85b This means that every instantiation of `LinuxAdmin::Rhn` will then add a deprecation warning to STDOUT. Previously, this would also be triggered even if you used the public class methods on `LinuxAdmin::SubscriptionManager` (the deprecation's suggested replacement), because under the hood it determine which to (`Rhn` or `SubscriptionManager`) by initializing each and calling their `registered?` methods. Furthermore, these means if `Rhn` is registered on the system in addition to the preferred `SubscriptionManager`, it the `LinuxAdmin::SubscriptionManager.validate_credentials` class method would actually end up calling `LinuxAdmin::Rhn.validate_credentials` instead because of the preference. * * * While this favoring should probably be refine to first look at the class the `method_missing` method is being called against, this fix simply favors `SubscriptionManager` over `Rhn` when deciding which class to use.

miq-bot · 2018-03-14T00:10:18Z

Some comments on commits NickLaMuro/linux_admin@ca6ca6e~...7adf1e5

spec/registration_system_spec.rb

⚠️ - 4 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
⚠️ - 8 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
⚠️ - 9 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

miq-bot · 2018-03-14T00:10:22Z

Checked commits NickLaMuro/linux_admin@ca6ca6e~...7adf1e5 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

An update to the previous commit that makes sure to favor attempting to use the calling class when setting the `registration_type` on any of the subclasses of `LinuxAdmin::RegistrationSystem` instead of whatever comes first. Adds a tiny amount of overhead, but most use cases shouldn't notice it... ever.

Debatable whether this is a good thing, of if we should keep it to remind us to remove it in the future.

When accessing @registration_type for the first time, it would be unassigned cause the following ruby warning to be thrown: lib/linux_admin/registration_system.rb:6: warning: instance variable @registration_type not initialized This prevents it by setting it to `nil` if it isn't set already.

NickLaMuro · 2018-03-14T00:27:47Z

Fixed Hakiri warnings in #201

carbonin · 2018-03-14T13:51:50Z

spec/registration_system_spec.rb

+  end
+
+  def stub_registered_to_system(*system)
+    allow_any_instance_of(LinuxAdmin::SubscriptionManager).to receive_messages(:registered? => system.include?(:sm))


Can we use something other than allow_any_instance_of here?

I think allow(LinuxAdmin::SubscriptionManager).to receive(:new).and_return(double("SubscriptionManager", :registered? => system.include?(:sm))) would do the job as well, right?

Ah, just saw this was only moved, thought this was new.

Yeah... I wasn't a fan of it myself, but I figured changing it now was not a good idea.

chrisarcand · 2018-03-14T14:16:19Z

@NickLaMuro I think this is going about it the wrong way (and what I say here probably should have just been done with the original deprecation in #198, I acknowledge)

"SubscriptionManager is preferred, and uses Rhn under the hood as the only exception to the rule of 'Rhn is deprecated'" creates the need for these weird hacks. SubscriptionManager should be able to stand up on itself and Rhn utilize SubscriptionManager as a deprecated crutch for however long it's needed.

That is to say, any logic that we need for SubscriptionManager found within Rhn should be moved to SubscriptionManager. Any methods that are now missing in Rhn from the move should emit a deprecation warning as expected and then depend on SubscriptionManager to do its job.

Make sense? Is this a reason this approach wouldn't work better than what's going on in this PR?

NickLaMuro · 2018-03-14T15:42:14Z

Make sense?

@chrisarcand To be honest, no, and I have reread your statement about 4-5 times to try to maybe get what you are saying (arguably might be a reading comprehension part on my end... but I digress). That said, I will try and answer:

So just doing some quick git research, the RegistrationSystem.registration_type_uncached has been in place for 5 years now:

fe3b39d7#diff-09e060ae786b7916c51fde7ebb8ae488R24

And looks like it was changed to favor Rhn shortly after:

d79f2948

With that background known, what it seems like you are suggesting is that we basically re-write this method_missing business, which seems very... risky, for the goal of this being basically a patch fix. If that is the end goal, I don't think I am the right person to be working on it.

I also feel like I am missing some context as to "why" Rhn is now deprecated, more specifically "now", but I can assume because SubscriptionManager has been the new tech for a while now and soon Rhn will be sunset (just my guess). Clearly Rhn was chosen to be the default at one point (see second commit linked above), but based on #198, it seems that doesn't make sense anymore, or even trying to keep the features of it around.

To me, it seems like SubscriptionManager can just completely replace Rhn AND the RegistrationSystem code entirely in v2.0 of linux_admin, pulling in the items into the base class, and we can then avoid this intermediate step that I think you are suggesting all together.

Again, that goes above and beyond the original intent of the PR, which was to remove deprecation warnings (and side-car'd in fixing some ruby warnings that I noticed in the process), because libraries should not emit warnings to consumers by just using them, only if they were used improperly. The MIQ case linked in the description is a situation where it was being used correctly, but a warning was still omitted, so I was trying to fix the oversight (which it seemed to be), not re-architect the entire codebase for RegistrationSystem classes.

NickLaMuro · 2018-03-14T15:48:34Z

To all:

For what it is worth, I don't mind splitting up or omitting pieces of what I wrote here. Fixing the deprecation warning when a user was using SubscriptionManager as suggested and still getting the warning was the main fix, and was "done" in the first commit. Everything after that was just extra.

chrisarcand · 2018-03-14T19:40:02Z

My comment was largely thinking in more general terms and now I see why in the code itself it's sort of lost. I'm not really understanding how registrations work and why that conditional has to be that way. @bdunne given your experience here do you see how one can decouple Rhn away entirely?

bdunne · 2018-03-14T19:44:14Z

We could just kill it completely and release v2.0.0 ¯_(ツ)_/¯

chrisarcand · 2018-03-14T19:50:46Z

Now you're talkin'.

chrisarcand · 2018-03-14T19:57:27Z

So the SubscriptionManager doesn't actually use Rhn at all, it really is just the check. From your reading description it didn't seem like it. We can do this change with @carbonin's feedback or just remove it as @bdunne said, whichever. Sorry for the noise.

NickLaMuro · 2018-03-14T20:33:24Z

@chrisarcand well, here is the thing: SubscriptionManager won't "use" Rhn's class for pieces that are missing in SubscrptionManager when calling .verify_credentials (the class method, not the instance method, which is handled by method_missing in the RegistrationSystem base class), it will straight up give you an instance of Rhn back (assuming that registered? returns true for Rhn). To me, that is a bug.

Specifically if there happens to be a system where both SubscriptionManager.new.registered? and Rhn.new.registered? returns true, and we are forced into using the deprecated codebase. In practice, this probably isn't the case, but in a test suite like ManageIQ, it becomes super cumbersome to stub out and work with both without knowing the internals of this lib to do so, which is something we shouldn't be passing on to other code bases.

So to summarize the first and second commits:

The first favors SubscriptionManager when making our opinionated decision to choose the RegistrationSystem for you, with out really fixing the "both are valid on this system case"
The second allows the user, despite our own opinions, to force a options when both are valid, but still fall back when to our order when their choice isn't valid.

I would argue that the first and second commits should both be merged to fix the bug, but arguably if we didn't want that much change for a patch release, I could see just merging the first commit.

That all said, if you peeps want to refactor this and "make it great again", in addition to or in favor of this PR, be my guest and I won't be offended in whichever route is pick.

But, I literally forked this yesterday, so if you think you can trick my statement of:

"hey, you have a bug in your code, and you should feel bad"

- me

with a counter of:

"no, YOU should feel bad because you didn't completely refactor it and make it the best it can be!"

- @chrisarcand totally said this word for word

I ain't falling for that one. Nope. Nope. Nuh-uh. Peace out.

jrafanie · 2018-03-15T18:54:16Z

While this solves the deprecation warning, I'm in favor of deleting it instead if that's something we can do, therefore I prefer #202

NickLaMuro force-pushed the more-rhn-deprecation branch from 0771d66 to 7adf1e5 Compare March 14, 2018 00:10

NickLaMuro added 3 commits March 13, 2018 19:11

Suppress Rhn warns in registration_system_spec

f955a0a

Debatable whether this is a good thing, of if we should keep it to remind us to remove it in the future.

carbonin reviewed Mar 14, 2018

View reviewed changes

chrisarcand mentioned this pull request Mar 14, 2018

Fix LinuxAdmin::Rhn deprecation warning... kinda ManageIQ/manageiq#17153

Closed

bdunne mentioned this pull request Mar 14, 2018

Drop LinuxAdmin::RegistrationSystem::Rhn #202

Closed

NickLaMuro closed this Mar 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prioritize SubscriptionManager over Rhn, and other warning fixes #200

Prioritize SubscriptionManager over Rhn, and other warning fixes #200

NickLaMuro commented Mar 13, 2018 •

edited

miq-bot commented Mar 14, 2018

miq-bot commented Mar 14, 2018

NickLaMuro commented Mar 14, 2018

carbonin Mar 14, 2018 •

edited

carbonin Mar 14, 2018

NickLaMuro Mar 14, 2018

chrisarcand commented Mar 14, 2018 •

edited

NickLaMuro commented Mar 14, 2018 •

edited

NickLaMuro commented Mar 14, 2018

chrisarcand commented Mar 14, 2018

bdunne commented Mar 14, 2018

chrisarcand commented Mar 14, 2018

chrisarcand commented Mar 14, 2018

NickLaMuro commented Mar 14, 2018 •

edited

jrafanie commented Mar 15, 2018

Prioritize SubscriptionManager over Rhn, and other warning fixes #200

Prioritize SubscriptionManager over Rhn, and other warning fixes #200

Conversation

NickLaMuro commented Mar 13, 2018 • edited

The fix

Links

miq-bot commented Mar 14, 2018

miq-bot commented Mar 14, 2018

NickLaMuro commented Mar 14, 2018

carbonin Mar 14, 2018 • edited

Choose a reason for hiding this comment

carbonin Mar 14, 2018

Choose a reason for hiding this comment

NickLaMuro Mar 14, 2018

Choose a reason for hiding this comment

chrisarcand commented Mar 14, 2018 • edited

NickLaMuro commented Mar 14, 2018 • edited

NickLaMuro commented Mar 14, 2018

chrisarcand commented Mar 14, 2018

bdunne commented Mar 14, 2018

chrisarcand commented Mar 14, 2018

chrisarcand commented Mar 14, 2018

NickLaMuro commented Mar 14, 2018 • edited

jrafanie commented Mar 15, 2018

NickLaMuro commented Mar 13, 2018 •

edited

carbonin Mar 14, 2018 •

edited

chrisarcand commented Mar 14, 2018 •

edited

NickLaMuro commented Mar 14, 2018 •

edited

NickLaMuro commented Mar 14, 2018 •

edited