Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

webpack:paths - always load inflectors #4205

Merged
merged 1 commit into from
Jun 25, 2018
Merged

webpack:paths - always load inflectors #4205

merged 1 commit into from
Jun 25, 2018

Conversation

himdel
Copy link
Contributor

@himdel himdel commented Jun 25, 2018

this was working when run as part of webpack:compile
but running webpack:paths by itself would fail to generate config/webpack/paths.json with manageiq-v2v, using manageiq-v2_v instead

@NickLaMuro please review :)

Copy link
Member

@NickLaMuro NickLaMuro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small suggestion. I was using invoke in the above since we had to do the Dir.chdir nonsense, but I think it makes more sense for this case to just do prereqs.

The alternative suggestion is just a convenience method, and not required.

@@ -92,6 +90,8 @@ namespace :webpack do
end

task :paths do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should just make this a prerequisite of the task using the normal mechanisms:

task :paths => ["ui:load_app_inflectors", "ui:load_asset_engine_inflectors"] do

Alternatively, we could also make a task under the :ui namespace that does this:

task :load_inflectors => ["ui:load_app_inflectors", "ui:load_asset_engine_inflectors"]

And then the above turns into:

task :paths => "ui:load_inflectors" do

@NickLaMuro
Copy link
Member

@himdel sorry about not doing this right the first time around. Thanks for the fix!

this was working when run as part of webpack:compile
but running webpack:paths by itself would fail to generate manageiq-v2v, using manageiq-v2_v instead
@himdel
Copy link
Contributor Author

himdel commented Jun 25, 2018

No problem, fixed, thanks :)

@miq-bot
Copy link
Member

miq-bot commented Jun 25, 2018

Checked commit https://github.com/himdel/manageiq-ui-classic/commit/d0e5b1ffdd9793c39c918ff432d424e993b67aca 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. 👍

Copy link
Member

@NickLaMuro NickLaMuro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. Looks good (though I am now officially saying this about my own code effectively...).

@Fryguy Fryguy merged commit 865a0ce into ManageIQ:master Jun 25, 2018
@Fryguy Fryguy added this to the Sprint 89 Ending Jul 2, 2018 milestone Jun 25, 2018
@Fryguy
Copy link
Member

Fryguy commented Jun 25, 2018

Merged, but as I'm thinking about the inflectors code, I'm thinking Vmdb::Inflections.load_inflections should handle engines directly, and then you don't need 2 tasks.

@NickLaMuro
Copy link
Member

@Fryguy sure. My original intent of this was to make this an isolated without needing anything from the main repo for ease of initial merge. As this becomes less of a "one off", I am completely fine with this being something that is given more thought and first class support in our plugins.

But again, Sir Hacky McHackHack (aka, me), signing off on this one for now.

@himdel himdel deleted the fix-paths branch June 26, 2018 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants