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

[Enhancement-3191] transport_enabled setting on an auth domain and authorizer may be unnecessary after transport client removal #3754

Conversation

prabhask5
Copy link
Contributor

Description

Remove the transport_enabled setting in config.yml and its related code in DynamicConfigModel.

Issues Resolved

#3191

Is this a backport? If so, please add backport PR # and/or commits #
No

Testing

No tests were made, all tests should run since the behavior should not change.

Check List

  • New functionality includes testing
  • New functionality has been documented
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Prabhas Kurapati <[email protected]>
Copy link

codecov bot commented Nov 20, 2023

Codecov Report

Merging #3754 (514ed8b) into main (5114bb7) will increase coverage by 0.18%.
The diff coverage is 50.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3754      +/-   ##
==========================================
+ Coverage   65.24%   65.43%   +0.18%     
==========================================
  Files         297      297              
  Lines       21129    21111      -18     
  Branches     3451     3446       -5     
==========================================
+ Hits        13786    13813      +27     
+ Misses       5643     5596      -47     
- Partials     1700     1702       +2     
Files Coverage Δ
...ch/security/securityconf/DynamicConfigModelV7.java 77.71% <100.00%> (-0.01%) ⬇️
...ch/security/securityconf/DynamicConfigModelV6.java 0.00% <0.00%> (ø)

... and 5 files with indirect coverage changes

@cwperks
Copy link
Member

cwperks commented Nov 27, 2023

@prabhask5 This change looks good to me, but is it backwards compatible. If someone uses the same config that includes transport_enabled as a setting will it read the config and ignore the value or will it fail to startup since there is an unknown config value? Can we add any logging in on startup to log that a removed setting is still being referenced in a config file and that it can be safely removed?

@stephen-crawford
Copy link
Contributor

Closing this for inaction. Please open a new PR if you choose to pursue this change further.

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.

3 participants