Shopify / maintenance_tasks

A Rails engine for queueing and managing data migrations.
MIT License
1.04k stars 76 forks source link

Add generator option to specify path to generate tasks in #1080

Closed guillermoap closed 2 weeks ago

guillermoap commented 2 months ago

This PR introduces a new tasks_path option to the Task Generator, allowing users to specify a custom path for generating task files. Currently the only namespacing available is inside the app/tasks/ folder which I think lacks some flexibility when organizing an app with different directories.

guillermoap commented 2 months ago

I have signed the CLA!

etiennebarrie commented 2 months ago

You can already pass a module in the task name:

$ bin/rails generate maintenance_tasks:task admin/update_posts
      create  app/tasks/maintenance/admin/update_posts_task.rb
      create  test/tasks/maintenance/admin/update_posts_task_test.rb

However it goes after app/tasks/maintenance/, whereas you're tweaking the beginning of the path: app/admin/tasks/maintenance/update_posts_task.rb.

The problem is that this does not respect Rails conventions. The first level of folders in app/ is ignored, after that it defines the constant name, which after inflection should be Tasks::Maintenance::UpdatePostsTask:

Shopify/maintenance_tasks/test/dummy $ bin/rails zeitwerk:check 
Hold on, I am eager loading the application.
expected file app/admin/tasks/maintenance/update_posts_task.rb to define constant Tasks::Maintenance::UpdatePostsTask, but didn't

Also since the constant isn't defined under the Maintenance module, it won't be loaded/found by MaintenanceTasks::Task.load_all.

guillermoap commented 2 months ago

I understand that it doesn't follow Rails conventions and that is kind of the behavior I needed. Should I close the PR or try and fix the loading issue?

Thanks for taking a look!

etiennebarrie commented 2 months ago

I'm not sure how you want this to work, so I can't say if it can make sense. How would you go about fixing the loading issue? Do you configure the application in a special way for this to work?

I was able to get it to work using this:

diff --git i/test/dummy/config/application.rb w/test/dummy/config/application.rb
index 4f864e4..c1ebd16 100644
--- i/test/dummy/config/application.rb
+++ w/test/dummy/config/application.rb
@@ -23,6 +23,8 @@ class Application < Rails::Application

     # Only include the helper module which match the name of the controller.
     config.action_controller.include_all_helpers = false
+    config.autoload_paths += Dir["app/admin/*"]
+    config.eager_load_paths += Dir["app/admin/*"]

     # Settings in config/environments/* take precedence over those specified
     # here.

This is outside of the Rails conventions that we want to support. I think your best bet is to add the generator to your app and tweak it to your liking. If you add a generator to lib/generators/maintenance_tasks/task_generator.rb in your app it overrides the one provided by the gem.

guillermoap commented 2 months ago

I'm not sure how you want this to work, so I can't say if it can make sense.

I want to be able to specify a different folder for the maintenance tasks to live in, without having to manually move the 2 files out of the base app/tasks each time I use the generator. We have a specific folder in our app where everything that is temporary and meant to be deleted eventually, lives. I used the admin namespace just because it was present in the tests, not because it necessarily reflects my usecase.

This is outside of the Rails conventions that we want to support. I think your best bet is to add the generator to your app and tweak it to your liking. If you add a generator to lib/generators/maintenance_tasks/task_generator.rb in your app it overrides the one provided by the gem.

Makes sense. I'll try that approach, thanks!