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

fix: prevent duplicate configuration lines in production.rb #349

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

uurcank
Copy link

@uurcank uurcank commented Sep 15, 2024

The generator was adding duplicate lines to production.rb each time it is run

  config.active_job.queue_adapter = :solid_queue
  config.solid_queue.connects_to = { database: { writing: :queue } }
  
  config.solid_queue.connects_to = { database: { writing: :queue } }

This PR refactors the generator to:

  1. Check if the line already exists before adding it.
  2. With this, generator logs to the terminal whether a new line was inserted along with gsub

@dhh
Copy link
Member

dhh commented Sep 15, 2024

Why are you running the generator more than once? I mean, I think it would be nice for it to be idempotent, but we gotta find a simpler way to do that than what's proposed.

@uurcank
Copy link
Author

uurcank commented Sep 16, 2024

This happens when upgrading from one version of another to generate new files added by newer versions. I guess once a stable version is out, it would not happen or if you never run the generator again.

@uurcank
Copy link
Author

uurcank commented Sep 16, 2024

perhaps we can include both in the replacement string like this

gsub_file Pathname(destination_root).join("config/environments/production.rb"),
      /(# )?config\.active_job\.queue_adapter\s+=.*/,
      "config.active_job.queue_adapter = :solid_queue\n  config.solid_queue.connects_to = { database: { writing: :queue } }"

@dhh
Copy link
Member

dhh commented Sep 16, 2024

I thought that was what we were already doing? Another option is just not to do a replacement if it's already set to solid_queue.

@uurcank
Copy link
Author

uurcank commented Sep 16, 2024

Just to clarify the generator adds these two lines

config.active_job.queue_adapter = :solid_queue
config.solid_queue.connects_to = { database: { writing: :queue } }

but if generator runs again it ends adding another line because replacement string does not contain the whole thing

config.active_job.queue_adapter = :solid_queue
config.solid_queue.connects_to = { database: { writing: :queue } }

config.solid_queue.connects_to = { database: { writing: :queue } }

so this PR proposes to check separately whether config.solid_queue.connects_to = { database: { writing: :queue } } is present. Because config.active_job.queue_adapter may still be set to solid_queue but the second line could be missing.

I thought we could do all in a single replacement string (without +) but that still does not check if config.solid_queue.connects_to is there or not. So without using File.read we can check like this and it seems to be working

 # Replace or set `config.active_job.queue_adapter`
    gsub_file Pathname(destination_root).join("config/environments/production.rb"),
    /config\.active_job\.queue_adapter\s+=.*/,
    "config.active_job.queue_adapter = :solid_queue"

    # Inject `config.solid_queue.connects_to` if not already present
    gsub_file Pathname(destination_root).join("config/environments/production.rb"),
    /^\s*config\.solid_queue\.connects_to\s+=\s+\{.*\}\n/,
    '',
    verbose: false # If found, do nothing

    inject_into_file Pathname(destination_root).join("config/environments/production.rb"),
    "\n  config.solid_queue.connects_to = { database: { writing: :queue } }",
    after: /config\.active_job\.queue_adapter\s+=.*/

will push this shortly.

@Shaglock
Copy link

Hi DHH and @uurcank ,

I've encountered this issue as well while experimenting with Enlitenment and running their generators multiple times with different configurations. Initially, I didn't realize it was coming from Rails.

I believe the solution using File.read is robust and a bit more readable. Here's my suggestion that may be a little bit more readable. What do you think?

In any case, both solutions work fine for me as long as they are added. :)

  # Inject `config.solid_queue.connects_to` if not already present
  unless File.read(production_rb). include?("config.solid_queue.connects_to")
    inject_into_file production_rb, 
      "\n  config.solid_queue.connects_to = { database: { writing: :queue } }", 
      after: /config\.active_job\.queue_adapter\s+=.*/
  end

Thanks!

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