-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Refactor configmapprovider.Provider and Retrieved interfaces #4403
Refactor configmapprovider.Provider and Retrieved interfaces #4403
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4403 +/- ##
==========================================
- Coverage 90.31% 90.30% -0.02%
==========================================
Files 177 177
Lines 10338 10343 +5
==========================================
+ Hits 9337 9340 +3
- Misses 779 782 +3
+ Partials 222 221 -1
Continue to review full report at Codecov.
|
// TODO: see if this can be eliminated. | ||
type ChangeEvent struct { | ||
// Error is nil if the config is changed and needs to be re-fetched using Get. | ||
// It is set to configsource.ErrSessionClosed if Close was called. |
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.
Not sure we need this. When Close is called we can simply just not call the onChange
callback. We just need to ensure that we will not call the callback after Close returns.
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.
Yes, see my TODO comment above. Going to look into that separately.
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.
Perhaps we get rid of Error
field but keep ChangeEvent
for the possibility to extend in the future without breaking the interface.
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.
We still need an "error" for the case when the watcher fails something like an "errorHandler" for the watcher. We either have to funcs "onChange" and "onError" or we have this and if err != nil
probably we should close the service? Because I don't know how to recover similar with the Host.ReportError()?
So I would keep for the moment the error.
3e26c33
to
3d76ad1
Compare
This uses a callback instead of a blocking WaitForUpdate function to notify about config changes. This simplifies the usage of the Provider, especially when watching for changes is required. A follow up PR will add watching capabilities to existing Provider implementations.
3d76ad1
to
67c1968
Compare
// TODO: see if this can be eliminated. | ||
type ChangeEvent struct { | ||
// Error is nil if the config is changed and needs to be re-fetched using Get. | ||
// It is set to configsource.ErrSessionClosed if Close was called. |
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.
We still need an "error" for the case when the watcher fails something like an "errorHandler" for the watcher. We either have to funcs "onChange" and "onError" or we have this and if err != nil
probably we should close the service? Because I don't know how to recover similar with the Host.ReportError()?
So I would keep for the moment the error.
This uses a callback instead of a blocking WaitForUpdate function
to notify about config changes.
This simplifies the usage of the Provider, especially when watching
for changes is required.
A follow up PR will add watching capabilities to existing Provider
implementations.