Skip to content
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

Closed
wants to merge 4 commits into from

Conversation

arielshaqed
Copy link
Contributor

@arielshaqed arielshaqed commented Mar 31, 2024

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.

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.
Copy link

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

@arielshaqed arielshaqed added the include-changelog PR description should be included in next release changelog label Mar 31, 2024
Copy link

github-actions bot commented Mar 31, 2024

🎊 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

Copy link

E2E Test Results - DynamoDB Local - Local Block Adapter

10 passed

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.
@arielshaqed arielshaqed requested a review from a team April 2, 2024 15:16
@N-o-Z
Copy link
Member

N-o-Z commented Apr 7, 2024

@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
Copy link
Contributor Author

@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 arielshaqed marked this pull request as ready for review April 8, 2024 09:50
@Jonathan-Rosenberg
Copy link
Contributor

@arielshaqed what's the reasoning behind this PR?

@arielshaqed
Copy link
Contributor Author

@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.)

@ozkatz
Copy link
Collaborator

ozkatz commented Apr 15, 2024

does this mean we’re closer to carving out a lakeFS Go client lib that is consumable by other projects?

@arielshaqed
Copy link
Contributor Author

@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.

Copy link

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.

@github-actions github-actions bot added the stale label May 26, 2024
Copy link

github-actions bot commented Jun 2, 2024

Closing this PR because it has been stale for 7 days with no activity.

@github-actions github-actions bot closed this Jun 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
include-changelog PR description should be included in next release changelog stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants