-
Notifications
You must be signed in to change notification settings - Fork 85
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
Mark previously deprecated SSL settings as obsolete #183
base: main
Are you sure you want to change the base?
Conversation
- SSL settings that were marked deprecated in version `3.15.0` are now marked obsolete, and will prevent the plugin from starting. - These settings are: - `ca_file`, which should be replaced by `ssl_certificate_authorities` - `keystore`, which should be replaced by `ssl_keystore_path` - `keystore_password`, which should be replaced by `ssl_keystore_password` - `keystore_type`, which should be replaced by `ssl_keystore_password` - `ssl`, which should be replaced by `ssl_enabled`
ced1b0b
to
e39d7ba
Compare
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.
The setup of ssl parameters seems a bit complex now:
def setup_ssl_params!
# Infer the value if neither the deprecate `ssl` and `ssl_enabled` were set
infer_ssl_enabled_from_hosts
end
def infer_ssl_enabled_from_hosts
return if original_params.include?('ssl_enabled')
@ssl_enabled = params['ssl_enabled'] = effectively_ssl?
end
def effectively_ssl?
return true if @ssl_enabled
hosts = Array(@hosts)
return false if hosts.nil? || hosts.empty?
hosts.all? { |host| host && host.to_s.start_with?("https") }
end
I think that boils down to just:
def setup_ssl!
return if original_params.include?('ssl_enabled')
@ssl_enabled = if @ssl_enabled
true
else
Array(@hosts).all? { |host| host && host.to_s.start_with?("https") }
end
params['ssl_enabled'] = @ssl_enabled
end
Though i'm not entirely sure what mutating params
does in this. In general the params
scope is kind of a mystery to me 😅
8fe5229
to
0a28120
Compare
0a28120
to
be18f6f
Compare
This was about as simple as I could get to:
|
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.
Looks great!
Over to @karenzone for doc review |
3.15.0
are now marked obsolete, and will prevent the plugin from starting.ca_file
, which should be replaced byssl_certificate_authorities
keystore
, which should be replaced byssl_keystore_path
keystore_password
, which should be replaced byssl_keystore_password
keystore_type
, which should be replaced byssl_keystore_password
ssl
, which should be replaced byssl_enabled
Relates: #179