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

Introduce suspenders:jobs generator #1147

Merged
merged 1 commit into from
Dec 6, 2023

Conversation

laicuRoot
Copy link
Contributor

Ports the existing generator to Suspenders 3.0.

Copy link
Contributor

@stevepolitodesign stevepolitodesign left a comment

Choose a reason for hiding this comment

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

Thank you for taking this one on! I recognize this is a draft, but wanted to share some initial thoughts.

lib/generators/suspenders/jobs_generator.rb Outdated Show resolved Hide resolved
lib/generators/suspenders/jobs_generator.rb Outdated Show resolved Hide resolved
test/generators/suspenders/jobs_generator_test.rb Outdated Show resolved Hide resolved
@laicuRoot laicuRoot force-pushed the suspenders-3-0-0-jobs-generator branch from 89ceac7 to 35ad96e Compare December 1, 2023 17:08
@laicuRoot laicuRoot marked this pull request as ready for review December 1, 2023 17:10
@laicuRoot laicuRoot requested a review from thiagoa as a code owner December 1, 2023 17:10
module ActiveJob
module Logging
class EnqueueLogSubscriber < LogSubscriber
define_method :enqueue, instance_method(:enqueue)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this doesn't work in Rails 7.1.

Have you tested the generator "manually" on a new application?

Copy link
Contributor Author

@laicuRoot laicuRoot Dec 2, 2023

Choose a reason for hiding this comment

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

Thank you for the comment @crackofdusk ! It has made me investigate more and try the generator in a new app.
It looks like it’s working, but please let me know if you think otherwise.

I took the following steps to test it:

  • I created a new Rails 7.1.2 App

  • I added the generator to this app.

  • I added the template active_job.rb

  • I ran in my terminal bin/rails generate jobs

  • This has generated the following

    gemfile  sidekiq
             run  bundle install from "."
    Bundle complete! 16 Gemfile dependencies, 85 gems now installed.
    Use `bundle info [gemname]` to see where a bundled gem is installed.
          create  config/initializers/active_job.rb
    Add default Procfile.dev
          create  Procfile.dev
  • I can see the sidekiq gem in the Gemfile, the active_job file in initializers, and the Procfile with the expected configuration.

  • Then I create a job - MyJob

I modified the initializer to:

require "active_job/logging"
require "active_job/log_subscriber"


# ActiveSupport::Notifications.unsubscribe("enqueue.active_job")
Rails.logger.info "Custom Active Job initializer is running"

module ActiveJob
  module Logging
    class EnqueueLogSubscriber < LogSubscriber
      puts "ActiveJob::Logging::EnqueueLogSubscriber"
      puts "I'm running"
      define_method :enqueue, instance_method(:enqueue)
    end
  end
end

ActiveJob::Logging::EnqueueLogSubscriber.attach_to(:active_job)

I ran rails console and I can see:

ActiveJob::Logging::EnqueueLogSubscriber
I'm running

and when I run the job MyJob.perform_later, I can see in the logs:

Custom Active Job initializer is running
[ActiveJob] Enqueued MyJob (Job ID: 02a76977-c7a1-47c6-8366-787c097b83f4) to Sidekiq(default)
[ActiveJob] ↳ (irb):1:in `<main>'
[ActiveJob] Enqueued MyJob (Job ID: 02a76977-c7a1-47c6-8366-787c097b83f4)

Let me know what you think 😃

I added the gem from this branch into the new rails app using:

group :development, :test do
  gem "suspenders", github: "thoughtbot/suspenders", branch: "suspenders-3-0-0-jobs-generator"
end

and run the generator.

bin/rails g suspenders:jobs

I got the same output as above.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for testing! I had a problem with this patch when upgrading an application from Rails 7.0 to 7.1. The error could have been caused by something else. For me it's enough if this generator works a new Rails application.

@stevepolitodesign stevepolitodesign mentioned this pull request Dec 1, 2023
17 tasks
Copy link
Contributor

@stevepolitodesign stevepolitodesign left a comment

Choose a reason for hiding this comment

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

We'll also want to update the README and NEWS too, before merging.

@laicuRoot laicuRoot force-pushed the suspenders-3-0-0-jobs-generator branch from 24138dd to 1717fd0 Compare December 5, 2023 18:26
Copy link
Contributor

@stevepolitodesign stevepolitodesign left a comment

Choose a reason for hiding this comment

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

Great work. Thank you!

test/generators/suspenders/jobs_generator_test.rb Outdated Show resolved Hide resolved
lib/generators/suspenders/jobs_generator.rb Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Here's the commit that introduced this for context.

38b530c

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you 🙂

Ports the existing generator to Suspenders 3.0.
@laicuRoot laicuRoot force-pushed the suspenders-3-0-0-jobs-generator branch from 69c3190 to bab99b1 Compare December 6, 2023 09:04
@laicuRoot laicuRoot merged commit cb5dff8 into suspenders-3-0-0 Dec 6, 2023
2 checks passed
stevepolitodesign added a commit that referenced this pull request Apr 24, 2024
Follow-up to #1147

The [introduction][] of `config/initializers/active_job.rb` was rendered
obsolete by [rails/rails#43390][].

Additionally, the following Rails 7 defaults match the [existing
configuration][], so there is no need to redeclare them.

```ruby
Rails.application.config.action_mailer.deliver_later_queue_name
=> nil

Rails.application.config.action_mailbox.queues.routing
=> nil

Rails.application.config.active_storage.queues.analysis
=> nil

Rails.application.config.active_storage.queues.purge
=> nil

Rails.application.config.active_storage.queues.mirror
=> nil
```

This is relevant because the next release of Suspenders will only
support `rails >= 7.0`.

[introduction]: 38b530c
[rails/rails#43390]: rails/rails#43390
[existing configuration]: https://github.com/thoughtbot/suspenders/blob/bd40e33a585891afba380a7884284f5accc003cf/lib/suspenders/generators/jobs_generator.rb#L19-L23
stevepolitodesign pushed a commit that referenced this pull request May 10, 2024
Ports the existing generator to Suspenders 3.0.
stevepolitodesign added a commit that referenced this pull request May 10, 2024
Follow-up to #1147

The [introduction][] of `config/initializers/active_job.rb` was rendered
obsolete by [rails/rails#43390][].

Additionally, the following Rails 7 defaults match the [existing
configuration][], so there is no need to redeclare them.

```ruby
Rails.application.config.action_mailer.deliver_later_queue_name
=> nil

Rails.application.config.action_mailbox.queues.routing
=> nil

Rails.application.config.active_storage.queues.analysis
=> nil

Rails.application.config.active_storage.queues.purge
=> nil

Rails.application.config.active_storage.queues.mirror
=> nil
```

This is relevant because the next release of Suspenders will only
support `rails >= 7.0`.

[introduction]: 38b530c
[rails/rails#43390]: rails/rails#43390
[existing configuration]: https://github.com/thoughtbot/suspenders/blob/bd40e33a585891afba380a7884284f5accc003cf/lib/suspenders/generators/jobs_generator.rb#L19-L23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants