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

allow lakectl local to be "git data" #7618

Merged
merged 9 commits into from
Apr 3, 2024

Conversation

ozkatz
Copy link
Collaborator

@ozkatz ozkatz commented Apr 1, 2024

Allow symlinking lakectl --> git-data which would trigger lakectl local as git data.

This enables calling:

$ git data clone ...
$ git data status
$ git data commit -m "..."

And so on.

Set up:

lakectl local install-git-plugin ~/bin  # or any other location in users' path

Will put a symlink called git-data into that path. Calling lakectl by that name will automatically execute the lakectl local subcommand. By default, calling git <subcommand> where <subcommand> isn't an existing git command will execute git-<subcommand>.

@ozkatz ozkatz added improvement include-changelog PR description should be included in next release changelog minor-change Used for PRs that don't require issue attached labels Apr 1, 2024
Copy link

github-actions bot commented Apr 1, 2024

E2E Test Results - DynamoDB Local - Local Block Adapter

10 passed

Copy link

github-actions bot commented Apr 1, 2024

♻️ PR Preview 2442446 has been successfully destroyed since this PR has been closed.

🤖 By surge-preview

@ozkatz ozkatz marked this pull request as ready for review April 2, 2024 06:05
Copy link
Member

@N-o-Z N-o-Z left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All in all looks good!
Can we possibly add tests for the new command? We are already testing lakectl local with the git integration in esti - so we have an env to test it

Args: cobra.ExactArgs(1),
Run: func(cmd *cobra.Command, args []string) {
installDir := args[0]
if strings.HasPrefix(installDir, "~/") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're already using homedir.Expand in our code - you can use it here too

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if !info.IsDir() {
DieFmt("%s: not a directory.\n", installDir)
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we also want to verify it's in the path?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's possibly error prone: git is sometimes a script that sets up environment variables before calling the executable, so we can't guarantee the current $PATH is the one used when executing.


var localInstallGitPluginCmd = &cobra.Command{
Use: "install-git-plugin <directory>",
Short: "set up `git data` (directory must exist and be in $PATH)",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a long description here? I think in this case it's important to describe exactly how to use it

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added!

ctx := context.Background()

var cmd *cobra.Command
baseName := getBasename()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: might want to extract that to a "setupLocalCommand" function

@ozkatz ozkatz requested a review from N-o-Z April 2, 2024 16:27
@ozkatz
Copy link
Collaborator Author

ozkatz commented Apr 2, 2024

@N-o-Z added tests, not sure why Esti is completely failing now :( would appreciate some help

@N-o-Z
Copy link
Member

N-o-Z commented Apr 2, 2024

@N-o-Z added tests, not sure why Esti is completely failing now :( would appreciate some help

I wouldn't say completely :) just the new test that was added. Also needed to generate the cli docs.
Added a new commit, let's see if it works now

Copy link
Member

@N-o-Z N-o-Z left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the test!

Copy link
Contributor

@talSofer talSofer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice!!
Can you please update https://docs.lakefs.io/howto/local-checkouts.html so that it includes the option to use git data?

@talSofer talSofer self-requested a review April 3, 2024 08:34
@ozkatz ozkatz force-pushed the improvement/lakectl-local-git-data branch from 013b427 to 2fdb65f Compare April 3, 2024 09:24
@ozkatz ozkatz force-pushed the improvement/lakectl-local-git-data branch from 2fdb65f to 6365141 Compare April 3, 2024 10:56
@ozkatz ozkatz merged commit fdc89d2 into master Apr 3, 2024
36 checks passed
@ozkatz ozkatz deleted the improvement/lakectl-local-git-data branch April 3, 2024 12:27
ozkatz added a commit that referenced this pull request Apr 10, 2024
emulatorchen pushed a commit to emulatorchen/lakeFS that referenced this pull request May 27, 2024
emulatorchen pushed a commit to emulatorchen/lakeFS that referenced this pull request May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement include-changelog PR description should be included in next release changelog minor-change Used for PRs that don't require issue attached
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants