-
Notifications
You must be signed in to change notification settings - Fork 331
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
Make lakeFS multimodule: move logging, cache, testutil.Must to new lakefs/util module #7616
Conversation
See the [tutorial](https://go.dev/doc/tutorial/workspaces) on multi-module workspaces. By moving logging to a sub-module we isolate its dependencies without the multi-repository hassle. This change _does_ break Go dependencies. However we do not guarantee semver for our Go modules. Future commits will add more base utilities to module github.com/treeverse/lakefs/util.
No linked issues found. Please add the corresponding issues in the pull request description. |
🎊 PR Preview fbd35fe has been successfully built and deployed to https://treeverse-lakeFS-preview-pr-7616.surge.sh 🕐 Build time: 0.02s 🤖 By surge-preview |
Also move testutil.Must* from lakefs/pkg/testutil to lakefs/util/testutil. We will probably want to break up lakefs/pkg/testutil, it is a fairly random assembly of utilities used in testing.
4977851
to
fbd35fe
Compare
@arielshaqed I looked at the PR but I feel we should have a discussion on this with a larger audience. |
Thanks! I would expect this audience to congregate on a PR that does it. In any case, now that it's not entirely unacceptable, I'm moving it out of "Draft" and posting it to #dev.
Regarding test logic: I think that this refers to But this is definitely a comment to discuss during a review: what advantages does such a separation of modules give us? |
@arielshaqed what's the reasoning behind this PR? |
Mark safe-to-reuse packages from lakeFSThese are useful packages: we already use them in several other locations. I want to indicate particularly safe packages to use. Control dependencies of these general packagesAs a by-product, this way prevents "small" changes that suddenly bring in large portions of the lakeFS code. (For instance, IIRC we've had versions where logging/ ended up depending on portions of Graveler, just because they didn't log. And we discovered those when we suddenly got import loops.) |
does this mean we’re closer to carving out a lakeFS Go client lib that is consumable by other projects? |
@ozkatz :
It would be possible either way. This PR should make it easier, because it breaks down some dependency walls. |
This PR is now marked as stale after 30 days of inactivity, and will be closed soon. To keep it, mark it with the "no stale" label. |
Closing this PR because it has been stale for 7 days with no activity. |
What
Make lakeFS a multi-module workspace.
Here's a brief tutorial on multi-module workspaces. In brief, this lets us create a small module github.com/treeverse/lakefs/util that can be included without all the dependencies of github.com/treeverse/lakefs.
Put logging, cache, and testutil.Must* into the util/ module.
Tips for reviewing