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.

Sign up for GitHub

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

Jump to bottom

Move with_dummy_database... around the implementation of webpack:compile, not the call #2810

Merged
merged 1 commit into from Nov 30, 2017
Merged

Move with_dummy_database... around the implementation of webpack:compile, not the call #2810

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 10 additions & 19 deletions lib/tasks/manageiq/ui_tasks.rake
View file
Open in desktop
Expand Up @@ -14,22 +14,7 @@ namespace :update do
end
end

task :ui do
Rake::Task['update:bower'].invoke
Rake::Task['update:yarn'].invoke

# When available, run the `webpack:compile` tasks without a fully loaded
# environment, since when doing an appliance/docker build, a database isn't
# available for the :environment task (prerequisite for
# 'webpacker:compile') to function.
if defined?(EvmRakeHelper)
EvmRakeHelper.with_dummy_database_url_configuration do
Rake::Task['webpack:compile'].invoke
end
else
Rake::Task['webpack:compile'].invoke
end
end
task :ui => ['update:bower', 'update:yarn', 'webpack:compile']
end

namespace :webpack do
Expand All @@ -41,9 +26,15 @@ namespace :webpack do

[:compile, :clobber].each do |webpacker_task|
task webpacker_task do
Dir.chdir ManageIQ::UI::Classic::Engine.root do
Rake::Task["webpack:paths"].invoke
Rake::Task["webpacker:#{webpacker_task}"].invoke
# Run the `webpack:compile` tasks without a fully loaded environment,
# since when doing an appliance/docker build, a database isn't
# available for the :environment task (prerequisite for
# 'webpacker:compile') to function.
EvmRakeHelper.with_dummy_database_url_configuration do
Dir.chdir ManageIQ::UI::Classic::Engine.root do
Rake::Task["webpack:paths"].invoke
Rake::Task["webpacker:#{webpacker_task}"].invoke
end
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

@himdel himdel Nov 27, 2017 •
edited

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 :).

end
end
end
Expand Down