-
Notifications
You must be signed in to change notification settings - Fork 71.9k
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
diasend/camaps cloud sync #7820
Conversation
…ing processed multiple times
One part of refactor worth mention is, the plugins need to be split to separate client and server plugins, so plugins with functionality on both sides don't pull in unnecessary dependencies into the client. |
Ping @burnedikt to you notice this. +1 to getting this merged into the mainline at some point. |
First of all, thanks for considering merging the diasend bridge upstream into nightscout. I wasn't aware there already was another PR attempting this so thanks for mentioning it here.
I am very much open to help integrating the diasend-bridge within nightscout. I've also noticed that there's quite some similarities between other 3rd party cloud provider integrations that work in a similar manner (in fact the diasend bridge's plugin code was built on the mmconnect plugin code). So having some kind of shared code or at least plugin architecture / interfaces to implement would definitely help. Can you share more about what idea's you already have in mind regarding the refactoring? |
Regarding your comment on the increased memory requirements for CI: The increased memory limit only applies to the testing stage, i.e. not for mere But to be completely fair, I haven't looked into specifically why the |
We get a lot of accidental PRs from user who are trying to merge changes to their own forks. I just closed the other PR as I'm pretty much 100% sure it's one of those. |
It's maybe worth noting that the dependencies of the diasend-nightscout bridge apparently break nightscout on nodejs version 10.x. See this issue. My assumption was that the officially supported nodejs versions for nightscout are 12.x, 14.x and 16.x as indicated in the package.json but just wanted to raise awareness that popular 3rd party installers like the one from xDrip will lead to a broken state. |
Are there any blockers for integrating into Regarding the future there are several implementations that would benefit from a single architecture and dependency management.
They should all reside in a single shared repo have their http library injected once, rather than each requiring their own http libraries. The plugin integration with Nightscout requires access to Nightscouts databus, and to setInterval. There are areas that deserve special consideration:
They are all identical in the sense that the overall workflow looks something like:
|
Nice work on this! Make sure to merge or rebase to Nightscout's |
I've put up a rebased version of this PR at #7872. Since a bunch of forks are already running of my fork's master branch, I didn't want to mess up their update paths (yet), hence why there's a rebased
I like the idea generally and it would have helped to kickstart my project if there was some base code available upon which to build the functionality. That being said, not sure if all the "bridge" authors have the time to rework their bridges to a shared structure so in a way, it's also a strength to not force them into one and the same schema. Also, although there is a shared core, each bridge will work a bit differently, e.g. on diasend, not everything is exposed via the API and some things need to be scraped off their website which leads to additional dependencies. One thing that would've definitely helped me personally would've been an existing (typed) client for nightscout's (internal) API (in the end I created my own), there was a bit of trial and error involved to figure out how to retrieve and store entries / treatments. |
Feel free to close this PR in favor of the new one. This architecture allows us to maintain and add features without requiring additions to Nightscout core code (#7983). I'd like to get Medtronic and Diasend implemented for inclusion into Nightscout. |
Since I'm not the author of this PR, I can not close it. But this should definitely be closed. Not only because #7872 supersedes this but also because even #7872 is obsolete by now due to the fact that diasend has been replaced / migrated to glooko which is an entirely different service and therefore the sync doesn't work anymore. |
Creating a place to discuss this.
Related: https://github.com/burnedikt/diasend-nightscout-bridge
#7810 (comment)
This looks really promising. We've been considering refactoring these; the current architecture implicitly multiplies the dependency tree which has grown hard to manage. I think it would be better to have one project that wraps all cloud vendors as a subdirectory with shared dependencies, retry logic, and can inject everything needed for Nightscout integration as a sidecar or plugin once: Dexcom Share, Medtronic Carelink, Glooko/OP5, Diasend/CAMAPS, Abbott Libre View, Tandem tconnectsync, Tidepool, self (another NS api).
In particular, it's very useful to have telemetry to help track login attempts and the collect more information about the behavior and performance of third party cloud providers. The typescript reads nicely, and I plan to use axios across the board. How would you feel about participating in this effort, or even expanding diasend-nightscout-bridge to accommodate some of these ideas? It's a bigger effort, but makes things easier over time, including making it easier to introduce a new vendor like this in the future.