-
Notifications
You must be signed in to change notification settings - Fork 357
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
Move with_dummy_database... around the implementation of webpack:compile, not the call #2810
Conversation
…ile, not the call This was a fix from #1887, except as it is now, it only works when called via `update:ui`, not when `webpack:compile` is being called directly. Moving to the task implementation.
Checked commit https://github.com/himdel/manageiq-ui-classic/commit/f9e77ef1dadf418ea1363e814f50449a9cf31605 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0 |
Taking a look, but there might have been. Honestly, I forget... we'll see what I remember after I take a look at this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me. Left some insight to why the original defined?
was there, but not sure it is needed. I think I put it there just so people could use both tasks, and wouldn't confused with it working or not.
Dir.chdir ManageIQ::UI::Classic::Engine.root do | ||
Rake::Task["webpack:paths"].invoke | ||
Rake::Task["webpacker:#{webpacker_task}"].invoke | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think my rational for the defined?
is that if you run the rake webpack:compile
from this repo (not manageiq
proper, and also not the rake app:webpack:compile
task), then the task wouldn't work, since it wouldn't load the codebase from manageiq
. That said, I haven't tried it recently to confirm if it is a problem or not.
If it is, to avoid duplication, you could possibly do this:
task_proc = lambda do
Dir.chdir ManageIQ::UI::Classic::Engine.root do
Rake::Task["webpack:paths"].invoke
Rake::Task["webpacker:#{webpacker_task}"].invoke
end
end
if defined?(EvmRakeHelper)
EvmRakeHelper.with_dummy_database_url_configuration { task_proc.call }
else
task_proc.call
end
But again, that is only if my theory about the rake webpack:compile
from this repo is true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea with the proc, thanks :). (I was actually thinking gaah, I wish I could add an anonymous function... didn't think of a proc.)
That said, I can verify that running rake webpack:compile
from manageiq-ui-classic
does load manageiq/lib/tasks/evm_rake_helper.rb
.
So.. I guess we don't need it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meh, I would then say you are fine (pun not intended).
That said, does this need to be backported at all? If so, I am hesitant to remove the defined?
so close to the build date. But if we are just pushing this to master, then I think it is okay to keep what you have here, and let things break out in the wild if they do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 added gaprindashvili/no
.. nothing is actually biting us here, I just had a note about it in my TODO :).
@NickLaMuro is this PR good to go? |
@mzazrivec yeah, I say ship it since it is not being backported. Any issues will probably be found in dev with this, and we can fix it then. |
This was a fix from #1887, except as it is now, it only works when called via
update:ui
, not whenwebpack:compile
is being called directly.Moving to the task implementation. (+ also covering
webpack:clobber
now)@NickLaMuro Take a look please? :)
I think
EvmRakeHelper
should always be available so we don't really need thedefined?
condition do we? Or was there a different reason?