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

add standardized config interface #40

Merged
merged 4 commits into from
Dec 8, 2023
Merged

Conversation

helpotters
Copy link
Contributor

No description provided.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@helpotters helpotters linked an issue Dec 5, 2023 that may be closed by this pull request
@helpotters
Copy link
Contributor Author

Started from the controller-extensions branch, assuming it'll be merged soon.

Also, this required some changes in readyset.rb and also #raw_query.

Still working on the "standardization" part. I would like to make it robust and provide an easy path to add other config options later on.

@helpotters helpotters self-assigned this Dec 5, 2023
@helpotters helpotters marked this pull request as ready for review December 7, 2023 20:17
@ethan-readyset ethan-readyset changed the base branch from main to controller-extensions December 7, 2023 23:50
Copy link
Contributor

@ethan-readyset ethan-readyset left a comment

Choose a reason for hiding this comment

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

A couple suggestions!

@database_selector = { delay: 2.seconds }
@database_resolver = Readyset::DefaultResolver
@database_resolver_context = nil
@database_url = ENV['READYSET_URL'] || default_connection_url
Copy link
Contributor

Choose a reason for hiding this comment

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

I actually don't think we'll need to take this as a parameter. I started looking into #36, and I think it's probably best just to integrate into rails via config/databases.yml, which means we can get access to ReadySet's connection info via Rails.configuration if we need to. But since we'll be using ActiveRecord::Base.connected_to, I think we actually won't need access to the raw connection string

# Fetch the 'primary_replica' details if available, otherwise fallback to default database
replica_config = config['primary_replica'] || config
# @return [Readyset::Configuration] Readyset's current configuration
def self.configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

What was the motivation to move this from the Readyset module into Configuration? I'm worried it could be a bit confusing to have the Configuration class store an instance of itself 🤔

I laid out a couple approaches in #33 -- what do you think?

@ethan-readyset
Copy link
Contributor

Oh and one other thought: I wonder if it would be possible/desirable to have our configuration live in Rails.configuration.readyset. We should do some research into other gems to see if their config lives in the main Rails config

@helpotters helpotters merged commit e002479 into controller-extensions Dec 8, 2023
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.

Add a Readyset.configure method that allows users to configure our gem
2 participants