Better inflector support for ui rake tasks and asset plugins #3986

NickLaMuro · 2018-05-24T00:42:06Z

This better supports the ActiveSupport::Inflectors that are set by both the main app and any asset engines that are pulled in by the webpack tasks.

The PR creates two new tasks in the :ui namespace:

rake ui:load_app_inflectors
rake ui:load_asset_engine_inflectors

The first loads the inflectors from the main application, and the second pulls in any inflectors from the asset_engines that are present. The asset_engine inflector task does assume the file lives in the config/initializers/inflections.rb, so that is sort of being set as the standard as part of this PR.

Also, minor addition: The asset_engines method is now memoized. Your welcome. #smugNickIsSmug

Links

This PR is to support the miq_v2v_ui_plugin rename to manageiq-v2v, which is currently in progress: Rename to manageiq-v2v manageiq-v2v#344

Steps for Testing/QA

When the aforementioned rename of miq_v2v_ui_plugin has completed, we should test rake update:ui from both this repo and the main repo to confirm everything is working as expected

Spoiler! I have already done this from the main repo with these, and seems to work as expected. That said, others should do the same.

NickLaMuro · 2018-05-24T00:43:32Z

@miq-bot assign @Fryguy

This should fix the issues you were having with the miq_v2v_ui_plugin rename. Let me know if this does the trick and the conventions that I am setting are all "gravy" with you.

cc @himdel (because I am sure you have some sort of opinion with this)

himdel · 2018-05-24T13:35:59Z

Thanks @NickLaMuro, once (if?) merged, I'll add this to the "backport the necessary webpack/engine switching bits to gaprindashvili" PR :).

himdel · 2018-05-24T13:38:55Z

lib/tasks/manageiq/ui_tasks.rake

-  all_engines = Rails::Engine.subclasses.each_with_object({}) do |engine, acc|
-    acc[engine] = engine.root.realpath.to_s
-  end
+  @asset_engines ||= begin do


looks like begin do is not a thing ;)

@himdel heh, looks like #smugNickisSmug got cocky a little to early. Didn't test with that change... go figure it is the one that breaks (facepalm).

Will push a chance in a second.

Loads the lib/vmdb/inflections.rb file from the main app to inject the inflectors into tasks that aren't loading the entire Rails environment. Allows the inflectors to be evaluated without loading the entire Rails environment, which causes issues when compiling assets in the appliance build. This gets ride of the gross hardcoding business of the ManageIQ inflector that was done previously as a stopgap.

Adds a task to load inflectors from asset engines that define them. This assumes the convention of `config/initializers/inflectors.rb` being the path where the we set the inflectors in our plugins.

This is now getting called a few times when doing something `update:ui`, so memoize it so we limit the number of filesystem checks we do.

miq-bot · 2018-05-25T15:59:36Z

Checked commits NickLaMuro/manageiq-ui-classic@40b76be~...ba77c31 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. ⭐

Fryguy · 2018-05-29T20:21:05Z

I am good with this for now, but I think it would be preferable if this lived in the core repo in some way. Perhaps Vmdb::Inflections and/or Vmdb::Plugins could own the knowledge? I know you are trying to avoid loading the environment, but from a pluggability standpoint, this is likely not the only repo that will bring their own assets and inflectors.

himdel · 2018-05-30T11:07:56Z

this is likely not the only repo that will bring their own assets and inflectors.

v2v is also bringing assets - they're using the asset pipeline for css
the same goes for manageiq-graphql I think.

(but Webpack 5 should have sane support for non-javascript packs, so maybe that will go away)

himdel · 2018-06-06T10:27:40Z

Not backported as part of #4071, but after that one gets in gaprindashvili, this one will backport cleanly.

So.. adding gaprindashvili/yes.

This will only be useful after ManageIQ/manageiq-v2v#344, right? Meaning not needed for today's build so not needing the transformation label quite yet?

NickLaMuro · 2018-06-06T14:49:17Z

@himdel yeah, that sounds right to me.

himdel · 2018-06-25T16:28:19Z

@NickLaMuro not sure if anything changed but ... the rake task doesn't inflect as intended:

$ be rake webpack:paths
$ cat config/webpack/paths.json
..
    "manageiq-v2_v": "/home/himdel/.rbenv/versions/2.4.3/lib/ruby/gems/2.4.0/bundler/gems/miq_v2v_ui_plugin-6ee8b26a6a40",
..

NickLaMuro · 2018-06-25T16:30:52Z

😭

WHYYYYYYYYYYYYY!!!1!

Okay... I will try and take a look in a bit...

himdel · 2018-06-25T16:33:32Z

Thanks! :)

Just verified that

--- a/lib/tasks/manageiq/ui_tasks.rake
+++ b/lib/tasks/manageiq/ui_tasks.rake
@@ -162,6 +162,9 @@ def asset_engines
 end
 
 def asset_engines_plugins_hash
+  ActiveSupport::Inflector.inflections do |inflect|
+    inflect.acronym("V2V")
+  end
   asset_engines.each_with_object({}) do |(engine, path), plugin_hash|
     # remove '::Engine' from the constant name, underscore it, and replace
     # backslashes with dashes

works just fine, so it must be something in how we load the inflectors.

himdel · 2018-06-25T16:50:21Z

@NickLaMuro Ok, never mind...

This is not ":load_asset_engine_inflectors doesn't work".
This is .. running webpack:paths manually, it doesn't get loaded.

Sorry for the confusion :)

NickLaMuro · 2018-06-25T16:59:27Z

Oh, interesting. I wonder if that should be added as a dependency to that task.

himdel · 2018-06-25T17:00:21Z

Or that :) I tried to fix this in #4205

But if this is needed for other stuff too..

NickLaMuro · 2018-06-25T17:02:22Z

I see, that is why I got pinged so much. Just saw that now. Will comment there.

Fryguy · 2018-06-25T19:58:08Z

I waited on merging the rename thinking this fixed it, so this is a problem now that the rename has been merged.

miq-bot assigned Fryguy May 24, 2018

himdel mentioned this pull request May 24, 2018

v2v in gaprindashvili #3963

Closed

27 tasks

himdel reviewed May 24, 2018

View reviewed changes

NickLaMuro force-pushed the better_inflector_support_for_ui_tasks branch from 8b3d1ae to 897ccdb Compare May 24, 2018 14:49

NickLaMuro added 3 commits May 25, 2018 10:49

Support manageiq asset plugin inflectors

3a061aa

Adds a task to load inflectors from asset engines that define them. This assumes the convention of `config/initializers/inflectors.rb` being the path where the we set the inflectors in our plugins.

Memoize asset_engines

ba77c31

This is now getting called a few times when doing something `update:ui`, so memoize it so we limit the number of filesystem checks we do.

NickLaMuro force-pushed the better_inflector_support_for_ui_tasks branch from 897ccdb to ba77c31 Compare May 25, 2018 15:50

NickLaMuro mentioned this pull request May 25, 2018

Rename to manageiq-v2v ManageIQ/manageiq-v2v#344

Merged

Fryguy merged commit f6128de into ManageIQ:master May 29, 2018

Fryguy added the enhancement label May 29, 2018

Fryguy added this to the Sprint 87 Ending Jun 4, 2018 milestone May 29, 2018

himdel mentioned this pull request Jun 5, 2018

[GAPRINDASHVILI] Backport Webpack related changes #4059

Merged

himdel added the gaprindashvili/yes label Jun 6, 2018

himdel mentioned this pull request Jun 6, 2018

[GAPRINDASHVILI] Rake task changes for update:ui #4071

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better inflector support for ui rake tasks and asset plugins #3986

Better inflector support for ui rake tasks and asset plugins #3986

NickLaMuro commented May 24, 2018 •

edited

Loading

NickLaMuro commented May 24, 2018

himdel commented May 24, 2018

himdel May 24, 2018

NickLaMuro May 24, 2018

miq-bot commented May 25, 2018

Fryguy commented May 29, 2018

himdel commented May 30, 2018

himdel commented Jun 6, 2018

NickLaMuro commented Jun 6, 2018

himdel commented Jun 25, 2018

NickLaMuro commented Jun 25, 2018

himdel commented Jun 25, 2018

himdel commented Jun 25, 2018 •

edited

Loading

NickLaMuro commented Jun 25, 2018

himdel commented Jun 25, 2018

NickLaMuro commented Jun 25, 2018

Fryguy commented Jun 25, 2018

Better inflector support for ui rake tasks and asset plugins #3986

Better inflector support for ui rake tasks and asset plugins #3986

Conversation

NickLaMuro commented May 24, 2018 • edited Loading

Links

Steps for Testing/QA

NickLaMuro commented May 24, 2018

himdel commented May 24, 2018

himdel May 24, 2018

Choose a reason for hiding this comment

NickLaMuro May 24, 2018

Choose a reason for hiding this comment

miq-bot commented May 25, 2018

Fryguy commented May 29, 2018

himdel commented May 30, 2018

himdel commented Jun 6, 2018

NickLaMuro commented Jun 6, 2018

himdel commented Jun 25, 2018

NickLaMuro commented Jun 25, 2018

himdel commented Jun 25, 2018

himdel commented Jun 25, 2018 • edited Loading

NickLaMuro commented Jun 25, 2018

himdel commented Jun 25, 2018

NickLaMuro commented Jun 25, 2018

Fryguy commented Jun 25, 2018

NickLaMuro commented May 24, 2018 •

edited

Loading

himdel commented Jun 25, 2018 •

edited

Loading