-
-
Notifications
You must be signed in to change notification settings - Fork 160
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
fix: gcp retriever is able get the updated flags file #311
fix: gcp retriever is able get the updated flags file #311
Conversation
- Remove internal file reader - Remove not need interface object - Improve unit tests avoiding the use of storage mocks
Don't worry about the linting issue, I will fix it in another PR and will works for you. |
Signed-off-by: Thomas Poignant <[email protected]>
Signed-off-by: Thomas Poignant <[email protected]>
Kudos, SonarCloud Quality Gate passed!
|
@thomaspoignant PR able to merge! 🚀 |
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.
Thanks a lot for you contribution @jferrl
Sorry for the delay in the review.
I will release a new version with this fix today.
Description
What was the problem?
It was not possible to use an instance of gcstorageretriever.Retriever{} since it is only able to get the file on the initial bootstrap of go-feature-flag instance. After feature flag client is up and running, the internal flags cache is no longer updated when gcstorageretriever is used.
How it is resolved?
The issue has been resolved avoiding the use of the file reader as a field of
Retriever
. So, after the first bootstrap ofRetriever
the file reader was closed because the code closes the reader after done reading. If we try out another call to Retriever.Retrieve() the reader is not nil and it was closed, so that, it was not able to get the updated file. In that case, we could see some log trace like:How can we test the change?
gcstorageretriever.Retriever{}
Changes include
Closes issue(s)
#310
Checklist