Adds MiqHelper #15020

NickLaMuro · 2017-05-05T22:43:23Z

Adds two methods to a global Miq module:

Miq.env: Functions the same as Rails.env, but falls back to an equivalent implementation when Rails isn't present
Miq.root: Functions the same as Rails.root, but falls back to an equivalent implementation when Rails isn't present

Tests are done in such a way that if Rails changes it's implementation that causes a difference in what we are seeing here, hopefully these tests will catch it.

Another support PR for #14916

Links

[WIP][POC] Slim (mini) evm rake tasks #14916

Fryguy · 2017-05-08T16:02:06Z

lib/miq_helper.rb

+require 'active_support/core_ext/object/blank'
+require 'active_support/string_inquirer'
+
+module Miq


Since we already have a ManageIQ directory and namespace, can we reuse that one?

lib/manageiq specifically...so I'm expecting this to be in lib/manageiq.rb (File name should match the class path)

Well, I went with lib/miq_helper.rb because I was matching lib/vmdb_helper.rb. Fine with changing it if you want. Just wanted to keep it as short as possible as well (otherwise, people will be less inclined to use it 😉 )

Ok, so lib/manageiq.rb with a ManageIQ module is probably the way to go.

This is a commit to help implement the features implemented in the following PRs: ManageIQ#14953 ManageIQ#14967 ManageIQ#15014 ManageIQ#15018 ManageIQ#15019 ManageIQ#15020 Meant to illustrate how they will eventually be used in this PR, and show how they will be implemented when they are merged into master. The prior commits to this one are left in place to show before and after, but this and those will be removed in the final iteration of this pull request, and only the necessary changes needed to implement this in the Rake tasks will be part of this PR. Applying all of the changes for this PR was done as follows: $ git apply <(curl -L ManageIQ#14953) $ git apply <(curl -L ManageIQ#14967) $ git apply <(curl -L ManageIQ#15014) $ git apply <(curl -L ManageIQ#15018) $ git apply <(curl -L ManageIQ#15019) $ git apply <(curl -L ManageIQ#15020) And then with some extra changes on top of that: * Making use of `Miq.env` and `Miq.root` where relevant * Implementing the modulularized PRs in the `tools/utils/mini_*.rb` files where relavant * Various bug fixes (though some seem like they shouldn't be there...) Again, this is meant as a demonstration commit to show the before and after while this is still a POC. Once the necessary pieces are in place on master to make this commit irrelevant, it will be rebased out. It may also change over time as this PR is worked on further.

jrafanie · 2017-05-31T20:31:09Z

spec/lib/miq_helper_spec.rb

+# If tests are not passing, check to see if the spec/spec_helper.rb is being
+# loaded properly and initailizing the Vmdb::Application.
+
+require 'miq_helper'


If you name it correctly, this is not needed since the constant lookup on line 11 would autoload it.

Not a fan, but clearly there is no pleasing you with less typing, so "long name it is"...

jrafanie · 2017-05-31T20:31:39Z

spec/lib/miq_helper_spec.rb

+    `#{Gem.ruby} -e 'require "#{miq_lib_file}"; print #{rb_cmd}'`
+  end
+
+  describe "::env" do


nit: describe ".env" do

jrafanie · 2017-05-31T20:31:57Z

spec/lib/miq_helper_spec.rb

+  end
+
+  describe "::env" do
+    before(:each) do


each is the default so 🔥 ✂️

This does a reset of the instance variable that is cached on the class (module) level, so reseting to a clean state makes sense for each test (even though it "might" not do anything).

jrafanie · 2017-05-31T20:33:07Z

spec/lib/miq_helper_spec.rb

+  end
+
+  describe "::root" do
+    before(:each) do


ditto on both lines

See https://github.com/ManageIQ/manageiq/pull/15020/files#r119523498

jrafanie

@NickLaMuro @Fryguy and I looked at this and other than the nitpicks below, it looks good. The followup PRs will need further review.

Adds two methods to a global ManageIQ module: ManageIQ.env: Functions the same as Rails.env, but falls back to an equivalent implementation when Rails isn't present ManageIQ.root: Functions the same as Rails.root, but falls back to an equivalent implementation when Rails isn't present Tests are done in such a way that if Rails changes it's implementation that causes a difference in what we are seeing here, hopefully these tests will catch it.

miq-bot · 2017-06-01T04:12:51Z

Checked commit NickLaMuro@50208b1 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

NickLaMuro · 2017-06-01T04:16:43Z

@Fryguy @jrafanie This and this:

Ok, so lib/manageiq.rb with a ManageIQ module is probably the way to go.

If you name it correctly, this is not needed since the constant lookup on line 11 would autoload it.

Is actually not even correct. Because we already define the module in app/models/manageiq/..., and those tend to get loaded very early in most of the application processes. In this case, it means "Rails" already knows about the module, and so it just tries calling the method on that module (which has nothing on it defined).

If you want to test, just check out this branch (or do the git apply method in the PR description), and run rspec spec/lib/manageiq_spec.rb to see what happens after you remove the require.

I will let you to battle to determine best course of action after knowing this, since I assume this was a major rational for your decision above (for want to change Miq to ManageIQ that is).

jrafanie · 2017-06-02T18:30:51Z

Is actually not even correct. Because we already define the module in app/models/manageiq/..., and those tend to get loaded very early in most of the application processes. In this case, it means "Rails" already knows about the module, and so it just tries calling the method on that module (which has nothing on it defined).

If you want to test, just check out this branch (or do the git apply method in the PR description), and run rspec spec/lib/manageiq_spec.rb to see what happens after you remove the require.

I will let you to battle to determine best course of action after knowing this, since I assume this was a major rational for your decision above (for want to change Miq to ManageIQ that is).

I'm "fine" with avoiding a namespace collision but was under the impression this code would be used outside of the full rails process most times, otherwise there wouldn't be a need to handle when Rails isn't defined. I don't see this being used that frequently to worry about having to spell out ManageIQ. Maybe I'm wrong?

jrafanie · 2017-06-02T18:31:39Z

@Fryguy are you good with this now? I can see Nick's point about the namespace collision but 🤷

NickLaMuro · 2017-06-02T18:34:54Z

but was under the impression this code would be used outside of the full rails process most times, otherwise there wouldn't be a need to handle when Rails isn't defined.

This "allows" you to use this outside of a Rails process, but this will replace a lot of code in places where Rails.root would normally be used, and shared between processes that use Rails and those that down (MiqServer being an example class for that).

[skip ci]

Fix some minor typos from #15020

NickLaMuro mentioned this pull request May 5, 2017

Modularize patches/database_configuration.rb #14953

Merged

Fryguy reviewed May 8, 2017

View reviewed changes

chessbyte assigned Fryguy May 8, 2017

chessbyte added core enhancement labels May 8, 2017

NickLaMuro mentioned this pull request May 12, 2017

Split the code out for MiqQueue into modules #14967

Closed

NickLaMuro added a commit to NickLaMuro/manageiq that referenced this pull request May 12, 2017

Apply PR ManageIQ#15020 patch

a8c8bfd

NickLaMuro mentioned this pull request May 20, 2017

[WIP] Slim Worker Runner #15174

Closed

10 tasks

NickLaMuro added a commit to NickLaMuro/manageiq that referenced this pull request May 23, 2017

Apply PR ManageIQ#15020 patch

bcf5686

jrafanie reviewed May 31, 2017

View reviewed changes

jrafanie requested changes May 31, 2017

View reviewed changes

NickLaMuro mentioned this pull request Jun 1, 2017

Add lib/workers/bin/run_single_worker.rb script #15132

Merged

NickLaMuro force-pushed the miq_helper branch from ae11924 to 7d3d2c7 Compare June 1, 2017 04:06

NickLaMuro force-pushed the miq_helper branch from 7d3d2c7 to 50208b1 Compare June 1, 2017 04:07

jrafanie approved these changes Jun 2, 2017

View reviewed changes

jrafanie merged commit 037d27e into ManageIQ:master Jun 5, 2017

jrafanie added this to the Sprint 62 Ending Jun 5, 2017 milestone Jun 5, 2017

jrafanie added a commit to jrafanie/manageiq that referenced this pull request Jun 5, 2017

Fix some minor typos from ManageIQ#15020

263ac2b

[skip ci]

bdunne added a commit that referenced this pull request Jun 5, 2017

Merge pull request #15302 from jrafanie/fix_typos

fc1b237

Fix some minor typos from #15020

NickLaMuro mentioned this pull request Jan 4, 2018

De-couple DescendantLoader from Rails #15460

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds MiqHelper #15020

Adds MiqHelper #15020

NickLaMuro commented May 5, 2017

Fryguy May 8, 2017

Fryguy May 8, 2017

NickLaMuro May 8, 2017

jrafanie May 31, 2017

jrafanie May 31, 2017

NickLaMuro Jun 1, 2017

jrafanie May 31, 2017

NickLaMuro Jun 1, 2017

jrafanie May 31, 2017

NickLaMuro Jun 1, 2017

jrafanie May 31, 2017

NickLaMuro Jun 1, 2017

jrafanie left a comment

miq-bot commented Jun 1, 2017

NickLaMuro commented Jun 1, 2017

jrafanie commented Jun 2, 2017

jrafanie commented Jun 2, 2017

NickLaMuro commented Jun 2, 2017

Adds MiqHelper #15020

Adds MiqHelper #15020

Conversation

NickLaMuro commented May 5, 2017

Links

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrafanie left a comment

Choose a reason for hiding this comment

miq-bot commented Jun 1, 2017

NickLaMuro commented Jun 1, 2017

jrafanie commented Jun 2, 2017

jrafanie commented Jun 2, 2017

NickLaMuro commented Jun 2, 2017