-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Introduce global settings #2532
Conversation
maciaszczykm
commented
Oct 31, 2017
•
edited
Loading
edited
- add settings manager
- add global settings endpoint
- add settings view
- add settings menu entry
- handle concurrent settings change error (dialog with save anyways and refresh options)
- merge settings during save to avoid any data loss
- fix tests
- add tests
- create service to retrieve specific settings values and use it across the app
- generate translations
- service refresh after settings save
- handle cluster name setting
- handle items per page setting
- remove bower leftovers
- fix caching problem
It is ready for an initial review. I have to fix few things in tests/annotations etc. |
src/app/backend/settings/manager.go
Outdated
func (sm *SettingsManager) SaveGlobalSettings(client kubernetes.Interface, s *api.Settings) error { | ||
cm, isDiff := sm.load(client) | ||
if isDiff { | ||
return errors.New("settings changed since last reload") |
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.
Use LocalizeError
method here so we can localize it in frontend.
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.
It is not displayed to the user but used to display "save anyways" dialog.
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 will comment it in the code.
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.
Done.
Overall LGTM. |
Codecov Report
@@ Coverage Diff @@
## master #2532 +/- ##
==========================================
- Coverage 59.07% 58.93% -0.14%
==========================================
Files 598 607 +9
Lines 13092 13268 +176
==========================================
+ Hits 7734 7820 +86
- Misses 5148 5234 +86
- Partials 210 214 +4
Continue to review full report at Codecov.
|
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.
LGTM