-
Notifications
You must be signed in to change notification settings - Fork 355
fix(pkg): isolates dependency with github.com/odigos-io/go-rtml #2734
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
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2734 +/- ##
==========================================
+ Coverage 77.04% 77.05% +0.01%
==========================================
Files 464 466 +2
Lines 49188 49199 +11
==========================================
+ Hits 37891 37903 +12
Misses 8507 8507
+ Partials 2790 2789 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
267aed3 to
34d3866
Compare
The introduction of "github.com/odigos-io/go-rtml" as a dependency has caused many go toolchain operations to fail because it now requires special flags to resolve a runtime symbol. This is not ideal because it breaks the developer experience for folks using IDEs with native Go support. Something as simple as go test ./... no longer works without flags. But most importantly: it becomes sticky to any go module importing SpiceDB. This commit isolates the memory protection usage by: - adding a global variable that acts as the registry - isolating the MemoryUsageProvider into its own package - introducing a new NoopMemoryLimiter, and using it everywhere we explicitly used the go-rtml package - moving tests into isolated packages - initializing the global default provider in main.go, using a function controlled by build tags. Without memoryprotection build tag, defaults to using the noop implementation. - moving all tests in the main package to make sure one can run them without the need of passing the special flags. Consequence: - now all tests run without special flags, except for those where we explicitly test for this behavior. - go modules importing SpiceDB do won't require the flags. - folks can run SpiceDB in their IDEs quickly without tinkering with toolchain flags
34d3866 to
97b8187
Compare
tstirrat15
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.
LGTM, but wondering about the function name.
Description
This PR isolates dependency with
github.com/odigos-io/go-rtmlThe introduction of
github.com/odigos-io/go-rtmlas a dependency has caused many go toolchain operations to fail because it now requires special flags to resolve a runtime symbol. This is not ideal because it breaks the developer experience for folks using IDEs with native Go support. Something as simple asgo test ./...no longer works without flags. But most importantly, it becomes sticky for any go module that imports SpiceDB.This commit isolates the memory protection usage by:
MemoryUsageProviderinto its own packageNoopMemoryLimiter, and using it everywhere we explicitly used the go-rtml packagemain.go, using a function controlled by build tags. Without thememoryprotectionbuild tag, it defaults to using the noop implementation.Consequence:
Testing
main.gowithout tag should show a warning indicating it fell back to noop providermain.gowithldflagsandmemoryprotectiontag, no warn log is printed, and log shows the type usedReferences