-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
feat: move Google config to installation_config #9482
base: develop
Are you sure you want to change the base?
Conversation
64a309e
to
aea1c67
Compare
config/initializers/omniauth.rb
Outdated
provider :google_oauth2, ENV.fetch('GOOGLE_OAUTH_CLIENT_ID', nil), ENV.fetch('GOOGLE_OAUTH_CLIENT_SECRET', nil), { | ||
provider_ignores_state: true | ||
} | ||
client_id = GlobalConfigService.load('GOOGLE_OAUTH_CLIENT_ID', nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sojan-official getting the values from GlobalConfigService
in this initializer, fails the CI test. You got ideas on how to fix this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P.S. Using this branch to test, #9504
I have removed the yarn step to speed up the CI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a possible solution here: https://github.com/chatwoot/chatwoot/pull/9504/files#diff-84ed1be2c135a8c628a3fb258e88c059ae24b3cb90abce9b5a5f74e96eb5c7ee
Not sure if it feels right. While the DB exists, the rescue case will not run. That means in production this will run as expected, while bypassing the issue in CI
LMK what you think @sojan-official
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pranavrajs @sojan-official here, the OmniAuth::Builder
does a lot more than just setting up the client, it also creates a Rack middleware stack by using the use
method inherited from Rack::Builder
. ref
Removing this seems non-trivial
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can look into lazyload hooks ?
ref: https://gorails.com/guides/rails-lazy-load-hooks-cheatsheet
https://edgeapi.rubyonrails.org/classes/ActiveSupport/LazyLoadHooks.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might have figured this one out, testing on staging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sojan-official this is resolved, PTAL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
False alarm, still figuring this out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like there's no way to do this, @sojan-official can you give this a shot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will take this forward
This PR has the following changes
GOOGLE_OAUTH_CLIENT_ID
andGOOGLE_OAUTH_CLIENT_SECRET
to installation configsuper_admin/features.yml
ENV.fetch
withGlobalConfigService.load
for fetch client ID and Secret