DRY up config for consumers of this gem #241
Conversation
benthorner
force-pushed
the
predefine-config
branch
3 times, most recently
from
de86cb4
to
49772fb
Compare
|
Tested switching to |
There was a problem hiding this comment.
This looks good, nice idea and approach 👍
I do wonder if we could go the whole hog and use the Rails.cache with this, as it feels a shame if we need to have the same GDS::SSO.config { |c| c.cache = Rails.cache } in most apps. I appreciate that's a bit of a pain though since it's not available at class parsing time. Though if we solve that (I recall you suggest a railstie which would do the trick) we could also clean up the api_only config which looks a bit logic heavy:
Lines 60 to 64 in 49772fb
| @@ -11,13 +12,15 @@ module Config | |||
|
|
|||
| # OAuth ID | |||
| mattr_accessor :oauth_id | |||
| @@oauth_id = ENV.fetch("OAUTH_ID", "test-oauth-id") | |||
There was a problem hiding this comment.
I feel like the environment variables are rather generically named - which was a pre-existing problem but feels a bit worse now you couldn't find usage in the codebase with a grep. It'd be more conventional for them to be prefixed with GDS_SSO_ but can understand that'd be a bit of puppet pain
There was a problem hiding this comment.
Yes, I agree the naming is short-sighted and could be more precise. I do think it's out-of-scope for the purpose of this change, since we don't currently have a clear conflict with these variables (until...accounts).
The naming doesn't seem to affect their discoverability, but they will become more hidden as a consequence of this change. I've added a couple of commits to refresh the README to help, which I also forgot to do earlier.
There was a problem hiding this comment.
Sure no worries and agree it was out of scope for here. While it's fresh in minds I think I'm going to have a go at switching them to have a GDS_SSO prefix - unless you have reason to object?
There was a problem hiding this comment.
Sure, that's fine with me. I guess it's not too hard to do, although you'll need to do a fair few app deployments!
benthorner
force-pushed
the
predefine-config
branch
from
2d5df7c
to
1b27bb9
Compare
Previously we required each consumer of this gem to have similar
initializers, with the following code:
GDS::SSO.config do |config|
config.user_model = "User"
config.oauth_id = ENV.fetch("OAUTH_ID", "abcdefg")
config.oauth_secret = ENV.fetch("OAUTH_SECRET", "secret")
config.oauth_root_url = Plek.new.external_url_for("signon")*
end
This updates the default config to match the common case for our
initializers, so that a given app only needs to specify additional
config, such as Rails caching.
Making this change removes a body of untested code that we would
otherwise need to consider testing before we could enable automatic
deployment for an app.
*Note that some apps use "Plek.find", which returns the internal
hostname for Signon. Since the external domain works for all apps,
this will switch all of them to use it.
This refreshes the installation instructions by removing the new defaults from the suggested initializer, consolidating the layout, and replacing the field list for the user table with a link to an example migration field with them all in.
benthorner
force-pushed
the
predefine-config
branch
from
1b27bb9
to
0bdad46
Compare
|
Tested switching to new initializer on email-alert-api locally:
|
|
@kevindew I was wondering the same. Since centralising and DRYing up config is the overall aim of this change, I had a go at automating the This works, and I've added a few more commits for it. |
benthorner
force-pushed
the
predefine-config
branch
from
0bdad46
to
1fdadd3
Compare
This is to call some attention to it being necessary to define the two environment variables.
Previously we required each API app to use efficient caching practices. This DRYs up the manual, repetitive caching config, so that we always cache bearer token responses unless configured otherwise.
Previously this involved a confusing mixture of two variables and conditionals. This simplifies the logic to be equivalent to writing the initializer inline, while still allowing it to be overridden.
benthorner
force-pushed
the
predefine-config
branch
from
1fdadd3
to
9ce3cdf
Compare
This was made redundant in [1]. [1]: alphagov/gds-sso#241
This was made redundant in [1]. [1]: alphagov/gds-sso#241
This was made redundant in [1]. [1]: alphagov/gds-sso#241
This was made redundant in [1]. [1]: alphagov/gds-sso#241
This was made redundant in [1]. [1]: alphagov/gds-sso#241
This was made redundant in [1]. [1]: alphagov/gds-sso#241
This was made redundant in [1]. [1]: alphagov/gds-sso#241
This was made redundant in [1]. [1]: alphagov/gds-sso#241
This was made redundant in [1]. [1]: alphagov/gds-sso#241
This was made redundant in [1]. [1]: alphagov/gds-sso#241
This was made redundant in [1]. [1]: alphagov/gds-sso#241
This was made redundant in [1]. [1]: alphagov/gds-sso#241
This was made redundant in [1]. [1]: alphagov/gds-sso#241
This was made redundant in [1]. [1]: alphagov/gds-sso#241
This was made redundant in [1]. [1]: alphagov/gds-sso#241
This was made redundant in [1]. [1]: alphagov/gds-sso#241
This was made redundant in [1]. [1]: alphagov/gds-sso#241
This was made redundant in [1]. [1]: alphagov/gds-sso#241
This was made redundant in [1]. [1]: alphagov/gds-sso#241
This was made redundant in [1]. [1]: alphagov/gds-sso#241
https://trello.com/c/epNWudCR/176-write-an-rfc-for-continuous-deployment-and-seek-review
Please see the commits for more details.