-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat: implement remote configuration and logging level updates #2498
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
base: development
Are you sure you want to change the base?
Conversation
443dc6e to
9b79f98
Compare
|
@coolwednesday and @aryanmehrotra kindly review the PR. |
Umang01-hash
left a comment
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.
-
Consider renaming interfaces for better clarity:
RemoteConfigurable→ ConfigSubscriber andRemoteConfiguration→ ConfigProvider -
All the newly added tests inside remotelogger_test.go have if conditions in the end like:
if atomic.LoadInt32(&logger.changeCnt) != 0 {
t.Errorf("expected no ChangeLevel when invalid key present")
} if !foundErrorLog {
t.Errorf("expected error log message for invalid JSON response, got logs: %v", logger.messages)
} if client.lastConfig["logLevel"] != "DEBUG" {
t.Errorf("expected logLevel=DEBUG, got %v", client.lastConfig["logLevel"])
}We should generally avoid all these if-else conditions inside test methods. Please refactor your tests too.
-
Same if-else conditions inside test lies in file r
emote_config_test.go. Please refactor it also. -
It would be nicer if you can also attach a screenshot of the functionality working after your changes so other developers can easily review.
|
|
||
| func fetchRemoteConfig(remoteService service.HTTP) (map[string]any, error) { | ||
| if newLogLevelStr, err := fetchLogLevelStr(remoteService); err != nil { | ||
| return map[string]any{}, err |
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.
Error handling in fetchRemoteConfig() should return nil, err instead of empty map on error
|
@ckshitij Thank you for contributing to GoFr. I have left some review comments on your PR. Please address them and we will review it again after all comments are resolved. |
|
@ckshitij Are you still working on this PR? If yes, let's resolve the review comments quickly so we can further review your PR. |
@Umang01-hash Yes I’m still working on the same, got busy with something , will try to address the comments by this weekend. |
- Added RemoteConfigurable and RemoteConfiguration interfaces for runtime configuration management. - Introduced mock implementations for testing remote configuration behavior. - Enhanced remote logger to support dynamic log level updates via remote configuration. - Implemented HTTP-based remote configuration fetching and client registration. - Added comprehensive tests for remote configuration and logger functionality.
…ndling (linting-issues)
5ff1550 to
27300a9
Compare
…ation to Subscriber and Provider for clarity
Pull Request
Description:
Checklist:
goimportandgolangci-lint.