-
Notifications
You must be signed in to change notification settings - Fork 71
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
feat: add 'watcher' interface to file sync #1365
Conversation
✅ Deploy Preview for polite-licorice-3db33c ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Hey @djosephsen, this seems like a good approach to me. @toddbaert @thisthat @Kavindu-Dodan do you have any concerns? |
Thanks @djosephsen for the awesome contribution, I am in favor of having this 👍 |
I agree with this approach! Thanks @djosephsen . I think the only thing I'm not sure about is how this configuration should be selected... Should we make it a "global" flag so that all file-watchers use a particular implementation? Should it allow it to be configured per-file-sync object? I think I favor the former... I think if someone wants to use a certain implementation, they will want to use it for all watchers, so a single global config makes sense. I also wonder if there's some way we can default to use this mode outside of K8s. Is there an "approved" means of knowing if you're running in K8s? Some env-var? The watcher as implemented works great there, since K8s behaves in very predictable ways wrt config-maps, but outside of that I think the new implementation will be more reliable (albeit a bit less performant). cc @thisthat @heckelmann on that last question. |
SGTM. Honestly making it a per-watcher switch hadn't even occurred to me.
I don't know how officially approved this is, but I've used |
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 like the approach and 👍 from my end. We could set a default operation mode (ex:- fsnotify
for historical reasons) and gradually move to fileinfo
. And document/recommend fileinfo
for situations where fsnotify
fails.
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, this is looking great! There's a few minor issues that should be addressed before merging but we're close.
When you get a moment, please:
- sign-off your commits. This is a requirement from the CNCF. Here's some information on how to easily update the entire branch.
- double-check the default "file" logic.
- add the new file syncs to the docs.
Thanks!
Just heads-up before this merges I should really flesh out fileinfo_watcher_test.go.. I feel like this is under-tested |
Sure, no problem. Please let us know when you're ready so we can rereview. Thanks! |
ok yall.. tests are fleshed out. Sorry that took a minute (writing tests is not my favorite thing). I have a question about how we want to handle the Error and Event channels given by the watchers. Currently we are using an unbuffered fsnotify .Watcher , but there is an upstream buffered option. Do we want to default to using unbuffered channels in the fileinfo watcher? My personal feeling is that buffered channels with sane defaults (size=32?) are pretty cheap, generally safer and less likely to lock the runtime in unexpected ways, unless you're genuinely holding something wrong. For example, In cases like the tests, where you're just kind of flexing the code-paths, unbuffered channels can create headaches. ATM I have silly values for both buffer sizes, and will change them to whatever yall think, at which point I think we're ready to merge pending your review. |
hmm, I just noticed I forgot the DCO again. Rebase is giving merge conflicts, and when I resolve them, giving me a commit message that looks like I might be overwriting someone else's history.. Do I need to close this and re-submit? (sorry my bad. I'll |
Hey @djosephsen, you should be able to run the following command: In your local branch, run: git rebase HEAD~10 --signoff Force push your changes to overwrite the branch: git push --force-with-lease origin main We'll squash merge ovce the pr is ready. |
Dco is a bit of a pain. We're looking into switching to a cla to simplify this process. |
Implement fsnotify and `os.Stat` based watchers fixes: open-feature#1344 Signed-off-by: Dave Josephsen <dave.josephsen@gmail.com>
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com> Signed-off-by: Dave Josephsen <dave.josephsen@gmail.com>
Signed-off-by: Dave Josephsen <dave.josephsen@gmail.com>
Signed-off-by: Dave Josephsen <dave.josephsen@gmail.com>
The first time I tried it went sideways, but the second time I was able to use some switch to skip an upstream merge, and that did it. |
@djosephsen if you want, you can approve or 👍 👎 here and you'll get an org invite. Then your CI tests will run automatically. There's no obligations involved in joining, but it's the first step on the contributor ladder. |
Signed-off-by: Dave Josephsen <dave.josephsen@gmail.com>
@djosephsen there're few lint issue due to unwrapped errors [1]. And you can run lint validations locally with [1] - https://github.com/open-feature/flagd/actions/runs/10406956145/job/28834819193?pr=1365#step:5:871 |
Hey hey... I think it might be idiomatically incorrect to wrap those errors. fsnotify_watcher is an interface wrapper, so by definition it doesn't add context to the upstream error. Totally happy to wrap them if ya want. But I think the correct move is probably to |
Yeah it doesn't add context to the upstream, but by wrapping we get more context for downstream (ex:- log analysis). I think this section of the checker explains why wrapping is recommended [1]. [1] - https://github.com/tomarrell/wrapcheck?tab=readme-ov-file#why |
Okeedokee, pr inbound |
Signed-off-by: Dave Josephsen <dave.josephsen@gmail.com>
Signed-off-by: Dave Josephsen <dave.josephsen@gmail.com>
I'm seeing a panic now when trying to run this locally. Error:
|
I saw that too.. I'll get it tracked down |
Signed-off-by: Dave Josephsen <dave.josephsen@gmail.com>
How we lookin? I can run the binary locally right now, but trying to run the integration tests, I don't have any of the files referenced by those commands.. like |
Hey @djosephsen, I'll manually test it real quick. If all looks good, I think we should merge. FYI @toddbaert |
It works great locally. Thank you very much @djosephsen. 🥳 |
🤖 I have created a release *beep* *boop* --- <details><summary>flagd: 0.11.2</summary> ## [0.11.2](flagd/v0.11.1...flagd/v0.11.2) (2024-08-22) ### 🐛 Bug Fixes * **deps:** update module buf.build/gen/go/open-feature/flagd/grpc/go to v1.5.1-20240215170432-1e611e2999cc.1 ([#1372](#1372)) ([ae24595](ae24595)) * **deps:** update module github.com/open-feature/flagd/core to v0.10.1 ([#1355](#1355)) ([8fcfb14](8fcfb14)) * **deps:** update module golang.org/x/net to v0.28.0 ([#1380](#1380)) ([239a432](239a432)) * **deps:** update module golang.org/x/sync to v0.8.0 ([#1378](#1378)) ([4804c17](4804c17)) ### 🧹 Chore * **deps:** update dependency go to v1.22.6 ([#1297](#1297)) ([50b92c1](50b92c1)) * **deps:** update golang docker tag to v1.23 ([#1382](#1382)) ([abb5ca3](abb5ca3)) * improve gRPC sync service shutdown behavior ([#1375](#1375)) ([79d9085](79d9085)) </details> <details><summary>flagd-proxy: 0.6.5</summary> ## [0.6.5](flagd-proxy/v0.6.4...flagd-proxy/v0.6.5) (2024-08-22) ### 🐛 Bug Fixes * **deps:** update module buf.build/gen/go/open-feature/flagd/grpc/go to v1.5.1-20240215170432-1e611e2999cc.1 ([#1372](#1372)) ([ae24595](ae24595)) * **deps:** update module github.com/open-feature/flagd/core to v0.10.1 ([#1355](#1355)) ([8fcfb14](8fcfb14)) * **deps:** update module golang.org/x/net to v0.28.0 ([#1380](#1380)) ([239a432](239a432)) * **deps:** update module golang.org/x/sync to v0.8.0 ([#1378](#1378)) ([4804c17](4804c17)) ### 🧹 Chore * **deps:** update dependency go to v1.22.6 ([#1297](#1297)) ([50b92c1](50b92c1)) * **deps:** update golang docker tag to v1.23 ([#1382](#1382)) ([abb5ca3](abb5ca3)) ### 📚 Documentation * **flagd-proxy:** removed invalid grpc prefix from uri config ([4911697](4911697)) </details> <details><summary>core: 0.10.2</summary> ## [0.10.2](core/v0.10.1...core/v0.10.2) (2024-08-22) ### 🐛 Bug Fixes * **deps:** update module buf.build/gen/go/open-feature/flagd/grpc/go to v1.5.1-20240215170432-1e611e2999cc.1 ([#1372](#1372)) ([ae24595](ae24595)) * **deps:** update module connectrpc.com/otelconnect to v0.7.1 ([#1367](#1367)) ([184915b](184915b)) * **deps:** update module github.com/open-feature/open-feature-operator/apis to v0.2.44 ([#1368](#1368)) ([0c68726](0c68726)) * **deps:** update module golang.org/x/crypto to v0.26.0 ([#1379](#1379)) ([05f6658](05f6658)) * **deps:** update module golang.org/x/mod to v0.20.0 ([#1377](#1377)) ([797d7a4](797d7a4)) * **deps:** update module golang.org/x/sync to v0.8.0 ([#1378](#1378)) ([4804c17](4804c17)) ### ✨ New Features * add 'watcher' interface to file sync ([#1365](#1365)) ([61fff43](61fff43)) * added new grpc sync config option to allow setting max receive message size. ([#1358](#1358)) ([bed077b](bed077b)) * Support blob type sources and GCS as an example of such source. ([#1366](#1366)) ([21f2c9a](21f2c9a)) ### 🧹 Chore * **deps:** update dependency go to v1.22.6 ([#1297](#1297)) ([50b92c1](50b92c1)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Signed-off-by: OpenFeature Bot <109696520+openfeaturebot@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Implement fsnotify and
os.Stat
based watchersfixes: #1344
This PR
Intent of this PR is to begin a conversation about fixing #1344. The approach taken is to replace the current use of
fsontify.Watcher
with a localWatcher
interface type that describes thefsnotify.Watcher
interface.My original take was to use fsnotify.Watcher directly as an implementation of local
Watcher
, but fsnotify's Watcher directly exposes its Error and Event channels, making it impossible to describe with an interface, so I had to create a small wrapper forfsnotify.Watcher
to satisfy the new Watcher interface (this is fsnotify_watcher.go). From there, we implement theWatcher
interface again, this time usingos.Stat
andfs.FileInfo
(this is fileinfo_watcher.go).Then we change the filepath sync code to use an interface to Watcher, rather than fsnotify.Watcher directly. The new fileinfo watcher plugs right in, and nothing really needs to change in the sync.
fs
that may require some work-around boilerplate to make this work on windows at the moment.If yall are favorable to this approach, I'll finish wiring up configs, and flesh out the tests. I didn't want to go much further without some buy-in or feedback.
Related Issues
Fixes #1344
Notes
See bullet-points above
How to test
go test -v ./...