Skip to content

allow cache to be an ActiveSupport::Cache object#43

Open
jdoconnor wants to merge 5 commits intofschuindt:masterfrom
jdoconnor:alllow-activesupport-cache
Open

allow cache to be an ActiveSupport::Cache object#43
jdoconnor wants to merge 5 commits intofschuindt:masterfrom
jdoconnor:alllow-activesupport-cache

Conversation

@jdoconnor
Copy link
Copy Markdown

@jdoconnor jdoconnor commented May 22, 2023

This appears to be functional in code. I'm going to be testing it for a while to see if I can get good backward compatibility.

todo

  • update comments in the code
  • update readme

@jdoconnor jdoconnor force-pushed the alllow-activesupport-cache branch from d139064 to 5822a04 Compare May 22, 2023 23:14
@jdoconnor
Copy link
Copy Markdown
Author

This should be a minor version bump the way it is.

There's a major version bump that changes the initialization interface that I will recommend.

Old way

FirebaseIdToken.configure do |config|
  config.redis = Redis.new
  config.project_ids = ['your-firebase-project-id']
end

new way

FirebaseIdToken.configure do |config|
  config.cache_store = ActiveSupport::Cache::RedisCacheStore.new
  config.project_ids = ['your-firebase-project-id']
end

If you're ready to make a major version bump, we can get rid of the backward compatible code at
https://github.com/fschuindt/firebase_id_token/pull/43/files#diff-9e6dda0d5f8ff3b88649df98ea8021c32ebca972799abdb0a055b5e4fb876dc8R64-R67

and the redis attr_accessor on the configuration
https://github.com/fschuindt/firebase_id_token/pull/43/files#diff-420fd33b48ab65deffa4060a1ce297e8d00721fa40db62e36b1477a005498c0eR7

@fschuindt
Copy link
Copy Markdown
Owner

@jdoconnor Can it be designed as an option, alongside Redis? So, the users can choose Redis or ActiveSupport::Cache. What are your thoughts regarding that?

@jdoconnor
Copy link
Copy Markdown
Author

it can be. I think it is a bit confusing, because the redis cache option allows for the same base functionality. I'll put that together as a friendlier way to migrate

and new `cache_store` configuration option
@jdoconnor
Copy link
Copy Markdown
Author

Thanks for the feedback.

The old config will work and operate like it did before.

The new configuration will use the fetch block, which eliminates the need to download certificates on a schedule.

@jdoconnor jdoconnor marked this pull request as ready for review May 23, 2023 19:47
@jdoconnor
Copy link
Copy Markdown
Author

closing temporarily for a bug I found

@jdoconnor jdoconnor closed this May 23, 2023
and DRYing up the class chooser based
on the configuration
@jdoconnor jdoconnor reopened this May 23, 2023
@jdoconnor jdoconnor force-pushed the alllow-activesupport-cache branch from f40f0d0 to 85a6320 Compare May 23, 2023 23:18
@jdoconnor
Copy link
Copy Markdown
Author

ready to be reviewed again. sorry for the back and forth

@stlucasgarcia
Copy link
Copy Markdown

Really nice PR! I am using the gem in production, and I would love to have that feature. Would you mind reviewing it, and possibly merge it @fschuindt if you have some free time?

I really appreciate this gem, it saved me a lot of time! Thanks for the work!

@jdoconnor
Copy link
Copy Markdown
Author

@lsglucas thanks for the kind words

@icaroryan
Copy link
Copy Markdown

Any updates on this PR?

end

def certificates=(value)
@certificates = klass
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Shouldn't this be @certificates = value? Otherwise when you do self.configuration.certificates = FirebaseIdToken::Testing::Certificates in self.test! the testing certificates won't actually be set?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

its been 2.5 years since I wrote this. If you want, I can try to re-gain the context and check or I'm happy to hand this off to someone more recently involved. Let me know

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm only looking at Firebase as a potential auth option, I'm not the right person to commit to something like this

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.

6 participants