lakeFS
lakeFS copied to clipboard
treeverse
Make lakeFS multimodule: move logging, cache, testutil.Must to new lakefs/util module
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.
- Ensure tests still run there.
Tips for reviewing
- The commits kinda make sense, you probably want to review by commit.
- Take a look at the result:
- Will it be easy to use logging/ and cache/ in their new locations?
- Is it worth doing in order to isolate their dependencies? Is util/go.mod an improvement?
- Historically there was some disagreement about whether multi-module workspaces should be used like this. What do you think?
- This PR breaks an implied guarantee about semantic version of Go modules. But not one of the guarantees that we made in the Towards 1.0 post. And this search shows no real uses of this repo.
No linked issues found. Please add the corresponding issues in the pull request description.
Use GitHub automation to close the issue when a PR is merged
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
🎊 PR Preview fbd35fe4c559b476b9b09e8bd2494f6971918fe9 has been successfully built and deployed to https://treeverse-lakeFS-preview-pr-7616.surge.sh
:clock1: Build time: 0.02s
🤖 By surge-preview
@arielshaqed I looked at the PR but I feel we should have a discussion on this with a larger audience. I went through the points you wrote and overall it seems to make sense but I would like to hear any objections if there are any to this change. Also - if we already created a new module, maybe it's better to separate the test logic and the utilities?
@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.
I went through the points you wrote and overall it seems to make sense but I would like to hear any objections if there are any to this change. Also - if we already created a new module, maybe it's better to separate the test logic and the utilities?
Regarding test logic: I think that this refers to util/testutil. Go specifically tends to mix test and other code, and I happen to like it. I am not sure that we want to create such small modules; NPM is over there if you want leftpad >:-) .
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 lakeFS
These 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 packages
As 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 :
does this mean we’re closer to carving out a lakeFS Go client lib that is consumable by other projects?
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.