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

Implement lakectl local list #6302

Merged
merged 3 commits into from
Aug 3, 2023
Merged

Implement lakectl local list #6302

merged 3 commits into from
Aug 3, 2023

Conversation

N-o-Z
Copy link
Member

@N-o-Z N-o-Z commented Aug 2, 2023

Linked Issue

Closes #6281


Change Description

Background

Implement lakectl local command which lists all folders linked to lakefs paths

New Feature

Part of task #6239

Testing Details

Unit testing for tools, lakectl tests will be added for esti upon feature completion

Breaking Change?

No


@N-o-Z N-o-Z added exclude-changelog PR description should not be included in next release changelog lakectl-local labels Aug 2, 2023
@N-o-Z N-o-Z self-assigned this Aug 2, 2023
@github-actions
Copy link

github-actions bot commented Aug 2, 2023

🎊 PR Preview 1af5109 has been successfully built and deployed to https://treeverse-lakeFS-preview-pr-6302.surge.sh

🕐 Build time: 0.014s

🤖 By surge-preview

@N-o-Z N-o-Z changed the title Task/lakectl local list 6281 Implement lakectl local list Aug 2, 2023
@N-o-Z N-o-Z force-pushed the task/lakectl-local-list-6281 branch from b82d4f4 to 89e5f05 Compare August 2, 2023 08:45
@N-o-Z N-o-Z requested a review from a team August 2, 2023 08:45
Copy link
Contributor

@nopcoder nopcoder left a comment

Choose a reason for hiding this comment

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

Added some comments about the code.
What is the use case of this command?
Was expected that we will have a command that will accept or use the current directory and lookup (in path or parents) for our yaml file and dump the information. Command like: lakectl local show.
Using find (command like utility) and lakectl local show I can run something like this list command.

Comment on lines +24 to +27
_, err := exec.LookPath("git") // assume git is in path, otherwise consider as not having git support
if err != nil {
return "", ErrNoGit
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm worried that there will be a long list of errors we would like to capture - why can we have one success and fail with all the rest?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are 2 types of errors we would like to specifically distinguish from which are the environment doesn't support git, and the current working directory is not a git repository.

Copy link
Contributor

Choose a reason for hiding this comment

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

You added lookup for git - so in the case we found git - we left only with the fact that that we have git tool and it didn't recognize a git repository.

Copy link
Member Author

Choose a reason for hiding this comment

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

Remove the generic GitError

return err
}
// don't traverse hidden folders like '.git', etc.
if d.IsDir() && d.Name()[0] == '.' {
Copy link
Contributor

Choose a reason for hiding this comment

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

strings.HasPrefix

Headers: []interface{}{
"Directory",
"Remote URI",
"Last synced HEAD",
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: "Synced Ref"

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the ref is too general. Should be either head or commit. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Commit sounds better

import (
"path/filepath"

"github.com/cockroachdb/errors"
Copy link
Contributor

Choose a reason for hiding this comment

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

"errors" from the std lib

@N-o-Z
Copy link
Member Author

N-o-Z commented Aug 3, 2023

Added some comments about the code. What is the use case of this command? Was expected that we will have a command that will accept or use the current directory and lookup (in path or parents) for our yaml file and dump the information. Command like: lakectl local show. Using find (command like utility) and lakectl local show I can run something like this list command.

This is basically the same functionality you described. When given a linked directory it will act exactly as 'show', and when providing a parent dir it will find all linked directories under that path

@N-o-Z N-o-Z requested a review from nopcoder August 3, 2023 08:10
Copy link
Contributor

@nopcoder nopcoder left a comment

Choose a reason for hiding this comment

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

LGTM - I'll move my functional comment to the design document.
Added comment in ReadIndex and optional suggestion for the column name.

}
idx := &Index{}
err = yaml.Unmarshal(data, idx)
idx.root = filepath.Dir(idxPath)
Copy link
Contributor

Choose a reason for hiding this comment

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

move after the err check

@N-o-Z N-o-Z enabled auto-merge (squash) August 3, 2023 14:47
@N-o-Z N-o-Z merged commit b925984 into master Aug 3, 2023
32 checks passed
@N-o-Z N-o-Z deleted the task/lakectl-local-list-6281 branch August 3, 2023 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exclude-changelog PR description should not be included in next release changelog lakectl-local
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement lakectl local list
2 participants