lakectl local: implement init #6280

N-o-Z · 2023-07-30T14:07:28Z

Linked Issue

Closes #6241

Change Description

Background

Implement first lakectl local command which initializes a local folder with a 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

github-actions · 2023-07-30T15:19:53Z

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

🕐 Build time: 0.021s

_{🤖 By surge-preview}

nopcoder · 2023-07-31T07:52:39Z

pkg/ioutils/io.go

We have pkg/fileutil for this kind of helpers

Not sure we need this one - the code uses it to validate if path (string) is directory but we actually need to verify if there is a file there or something else which means that we eat the error and assume it is a file.

Moved to fileutil
We check if the path is an existing dir in order to add the /* prefix to it.
This is a best effort as we cannot tell for the path string itself if it is a directory. In any other case (including if it doesn't exist) we want to return false.
I've update the code

nopcoder · 2023-07-31T08:01:23Z

pkg/git/git.go

General suggestion - package name is git so we can drop it from a lot of the function names.

nopcoder · 2023-07-31T08:03:12Z

pkg/git/git.go

+func git(dir string, args ...string) (int, string) {
+	stdout := new(strings.Builder)
+	stderr := new(strings.Builder)
+	cmd := exec.Command("git", args...)
+	cmd.Dir = dir
+	cmd.Stdout = stdout
+	cmd.Stderr = stderr
+	err := cmd.Run()
+	if err != nil {
+		var exitError *exec.ExitError
+		if errors.As(err, &exitError) {
+			return exitError.ExitCode(), stderr.String()
+		}
+		return -1, err.Error()
+	}
+	return 0, stdout.String()
+}


The cmd.CombinedOutput() should give you all you need - it will capture the stderr and stdout and we can use err is indicator of success as we do not associate any specific code to an error.

nopcoder · 2023-07-31T08:22:01Z

pkg/git/git.go

+// IsGitRepository Return true if dir is a path to a directory in a git repository, false otherwise
+func IsGitRepository(dir string) bool {
+	rc, _ := git(dir, "rev-parse", "--is-inside-work-tree")
+	return rc == 0
+}


Looks like we use it for testing only?
lakectl use it before calling git.Ignore but git.Ignore should verify the same.

It will be used in future PRs

nopcoder · 2023-07-31T08:22:13Z

pkg/git/git.go

+}
+
+// GetGitRepositoryPath Returns the git repository root path if dir is a directory inside a git repository, otherwise returns error
+func GetGitRepositoryPath(dir string) (string, error) {


RepositoryPath

cmd/lakectl/cmd/local_init.go

nopcoder · 2023-07-31T09:28:31Z

cmd/lakectl/cmd/local_init.go

+			if err != nil {
+				DieErr(err)
+			}
+			Fmt("location added to %s\n", ignoreFile)


Current location? we print out the ignore file which is already the same as our repository, right?

No - we don't know where the ignore file is relative to where we executed the command. It could be anywhere on the tree or it could have been created by us

cmd/lakectl/cmd/local_init.go

nopcoder · 2023-07-31T09:31:12Z

pkg/git/git.go

+	markerLine := "# " + marker
+	info, err := os.Stat(ignoreFilePath)
+	switch {
+	case err == nil: // ignore file exists
+		mode = info.Mode()
+		ignoreFile, err := os.Open(ignoreFilePath)
+		if err != nil {
+			return "", err
+		}
+		fileScanner := bufio.NewScanner(ignoreFile)
+		for fileScanner.Scan() {
+			line := strings.TrimSpace(fileScanner.Text())
+			ignoreContent = append(ignoreContent, line)
+			if line == markerLine {
+				found = true
+				for fileScanner.Scan() {
+					line = strings.TrimSpace(fileScanner.Text())
+					if line == "" {
+						ignoreContent = append(ignoreContent, "")
+						break
+					}
+					if !slices.Contains(ignoreEntries, line) {
+						ignoreContent = append(ignoreContent, line)
+					}
+				}
+				ignoreContent = append(ignoreContent, ignoreEntries...)
+			}
+		}
+
+		if !found { // Add the marker and ignore list to the end of the file
+			ignoreContent = append(ignoreContent, "")
+			ignoreContent = append(ignoreContent, markerLine)
+			ignoreContent = append(ignoreContent, ignoreEntries...)
+		}
+
+		err = ignoreFile.Close()
+		if err != nil {
+			return "", err
+		}
+	case !os.IsNotExist(err):
+		return "", err
+	default: // File doesn't exist
+		ignoreContent = append(ignoreContent, markerLine)
+		ignoreContent = append(ignoreContent, ignoreEntries...)
+	}
+
+	buffer := strings.Join(ignoreContent, "\n") + "\n"
+	if err = os.WriteFile(ignoreFilePath, []byte(buffer), mode); err != nil {
+		return "", err
+	}
+
+	return ignoreFilePath, nil


Can we split into 'readIgnore', 'updateIgnore' and 'writeIgnore'?
Think we should not mix the marker update and reading.

nopcoder · 2023-07-31T10:25:37Z

cmd/lakectl/cmd/local_init.go

+		}
+
+		if git.IsGitRepository(localPath) {
+			ignoreFile, err := git.Ignore(localPath, []string{localPath, IndexFileName}, []string{IndexFileName}, IgnoreMarker)


Do we need a flag to skip update of gitignore?

I'll open a different issue to explain the use-case I had in mind

nopcoder · 2023-07-31T10:56:07Z

pkg/git/git.go

+// If section exists, it will append paths to the given section, otherwise writes the sections at the end of the file.
+// File paths must be absolute.
+// Creates the .ignore file if it doesn't exist.
+func Ignore(dir string, ignorePaths, excludePaths []string, marker string) (string, error) {


general note - if we use io/fs (passing fs.FS to this call) we can abstract the os level and have the code test better without doing actual os level calls.

I don't think we can pass an FS object since the dir passed might not be the git repository root

nopcoder · 2023-08-01T13:43:29Z

cmd/lakectl/cmd/index.go

+type Index struct {
+	root    string
+	PathURI string `yaml:"src"`
+	Head    string `yaml:"at_head"`


Suggested change

Head string `yaml:"at_head"`

AtHead string `yaml:"at_head"`

cmd/lakectl/cmd/local_init.go

nopcoder · 2023-08-01T13:53:51Z

cmd/lakectl/cmd/local_init.go

+		}
+
+		if git.IsGitRepository(localPath) {
+			ignoreFile, err := git.Ignore(localPath, []string{localPath, IndexFileName}, []string{IndexFileName}, IgnoreMarker)


I'll open a different issue to explain the use-case I had in mind

nopcoder · 2023-08-01T13:54:45Z

cmd/lakectl/cmd/index.go

+// Index defines the structure of the lakefs local reference file
+// consisting of the information linking local directory with lakefs path
+type Index struct {
+	root    string


Suggested change

root string

root string `yaml:"-"`

nopcoder · 2023-08-01T16:31:09Z

cmd/lakectl/cmd/index.go

+		return false
+	default:
+		DieErr(err)
+		return false // go fmt


It's going to change in the next PR

nopcoder · 2023-08-01T16:37:49Z

cmd/lakectl/cmd/local_init.go

+	initMinArgs = 1
+	initMaxArgs = 2


generic name - need to add localInit or it may conflict with other geneic name soon

nopcoder · 2023-08-01T16:38:09Z

cmd/lakectl/cmd/local_init.go

+			dir = args[1]
+		}
+		flagSet := cmd.Flags()
+		force := MustBool(flagSet.GetBool("force"))


You can start using the generic Must

nopcoder · 2023-08-01T17:13:53Z

pkg/git/git.go

+func updateIgnoreFileSection(contents []byte, marker string, entries []string) []byte {
+	var newContent []byte
+	scanner := bufio.NewScanner(bytes.NewReader(contents))
+	for scanner.Scan() {
+		line := strings.TrimSpace(scanner.Text())
+		newContent = append(newContent, []byte(line+"\n")...)
+		if line == marker {
+			for scanner.Scan() {
+				line = strings.TrimSpace(scanner.Text())
+				if line == "" {
+					break
+				}
+				if !slices.Contains(entries, line) {
+					newContent = append(newContent, []byte(line+"\n")...)
+				}
+			}
+			buffer := strings.Join(entries, "\n") + "\n"
+			newContent = append(newContent, buffer...)
+		}
+	}
+
+	return newContent
+}


We need to use API which uses the OS's new-line or use the the right value for new-line when we write to a file

Did my best - golang doesn't have a definition for new line. Instead I used fmt.Sprintln()

…init-6241

nopcoder

Added optional suggestions.
My only concern is the marker range in the gitignore - we should consider using different marker for start and another for end to mark the section.
A user can update the ignore file and remove the blank line the next update will move part that is not of the marker.

nopcoder · 2023-08-02T06:36:20Z

cmd/lakectl/cmd/local_init.go

+			DieErr(err)
+		}
+		if IndexExists(localPath) && !force {
+			Die(fmt.Sprintf("directory '%s' already linked to a lakefs path, run command with --force to overwrite", localPath), 1)


nopcoder · 2023-08-02T06:37:48Z

cmd/lakectl/cmd/local_init.go

+		if err != nil && !errors.Is(err, git.ErrNotARepository) {
+			DieErr(err)
+		} else if err == nil {
+			fmt.Println("location added to", ignoreFile)
+		}


Alternative indent vs less 'if'

Suggested change

if err != nil && !errors.Is(err, git.ErrNotARepository) {

DieErr(err)

} else if err == nil {

fmt.Println("location added to", ignoreFile)

}

if err != nil {

if !errors.Is(err, git.ErrNotARepository) {

DieErr(err)

}

fmt.Println("location added to", ignoreFile)

}

Not exactly, but modified similarly

nopcoder · 2023-08-02T07:41:16Z

pkg/git/git.go

+func updateIgnoreFileSection(contents []byte, marker string, entries []string) []byte {
+	var newContent []byte
+	scanner := bufio.NewScanner(bytes.NewReader(contents))
+	for scanner.Scan() {
+		line := strings.TrimSpace(scanner.Text())
+		newContent = append(newContent, []byte(fmt.Sprintln(line))...)
+		if line == marker {
+			for scanner.Scan() {
+				line = strings.TrimSpace(scanner.Text())
+				if line == "" {
+					break
+				}
+				if !slices.Contains(entries, line) {
+					newContent = append(newContent, []byte(fmt.Sprintln(line))...)
+				}
+			}
+			buffer := strings.Join(entries, fmt.Sprintln("")) + fmt.Sprintln("")
+			newContent = append(newContent, buffer...)
+		}
+	}
+
+	return newContent
+}


Suggest the following to update/append the content:

func updateIgnoreFileSection(contents []byte, marker string, entries []string) ([]byte, error) { // Read content and remove/collect entries under marker var ( result bytes.Buffer inMarker bool scanner = bufio.NewScanner(bytes.NewReader(contents)) ) for scanner.Scan() { line := strings.TrimSpace(scanner.Text()) switch { case inMarker && line == "": inMarker = false case inMarker && !slices.Contains(entries, line): entries = append(entries, line) case !inMarker && line == marker: inMarker = true default: _, _ = fmt.Fprintln(&result, line) } } if err := scanner.Err(); err != nil { return nil, err } // Render marker and entries at the end _, _ = fmt.Fprintln(&result, marker) for _, entry := range entries { _, _ = fmt.Fprintln(&result, entry) } _, _ = fmt.Fprintln(&result) return result.Bytes(), nil }

Would prefer not to rewrite it to the end of the file for now. I will open another PR to also add an end marker

N-o-Z added exclude-changelog PR description should not be included in next release changelog lakectl-local labels Jul 30, 2023

N-o-Z requested review from nopcoder and a team July 30, 2023 14:07

N-o-Z self-assigned this Jul 30, 2023

lakectl local: implement init

9f86e01

N-o-Z force-pushed the task/lakectl-local-init-6241 branch from 77a59e8 to 9f86e01 Compare July 30, 2023 14:08

Gen docs

e4f6c91

Fix esti test

e83909b

nopcoder suggested changes Jul 31, 2023

View reviewed changes

nopcoder reviewed Jul 31, 2023

View reviewed changes

CR Fixes

f6cebf6

N-o-Z requested a review from nopcoder July 31, 2023 14:10

N-o-Z added 2 commits July 31, 2023 18:49

Gen docs

c1b3f63

Merge branch 'master' into task/lakectl-local-init-6241

bc2a2a3

nopcoder suggested changes Aug 1, 2023

View reviewed changes

Merge remote-tracking branch 'origin/master' into task/lakectl-local-…

4309d44

…init-6241

N-o-Z requested a review from nopcoder August 1, 2023 18:42

CR Fixes

305d1ec

N-o-Z force-pushed the task/lakectl-local-init-6241 branch from 0a85eae to 305d1ec Compare August 2, 2023 06:25

nopcoder approved these changes Aug 2, 2023

View reviewed changes

N-o-Z added 3 commits August 2, 2023 10:58

CR Fixes 2

bd0cd85

CR Fixes 3

d0e05a9

Gen docs

ed2c277

N-o-Z enabled auto-merge (squash) August 2, 2023 08:14

N-o-Z merged commit 8a5596b into master Aug 2, 2023
36 of 37 checks passed

N-o-Z deleted the task/lakectl-local-init-6241 branch August 2, 2023 08:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lakectl local: implement init #6280

lakectl local: implement init #6280

N-o-Z commented Jul 30, 2023 •

edited

Loading

github-actions bot commented Jul 30, 2023 •

edited

Loading

nopcoder Jul 31, 2023

N-o-Z Jul 31, 2023 •

edited

Loading

nopcoder Jul 31, 2023

nopcoder Jul 31, 2023

nopcoder Jul 31, 2023

N-o-Z Jul 31, 2023

nopcoder Jul 31, 2023

nopcoder Jul 31, 2023

N-o-Z Jul 31, 2023

nopcoder Jul 31, 2023

nopcoder Jul 31, 2023

N-o-Z Jul 31, 2023

nopcoder Aug 1, 2023

nopcoder Jul 31, 2023

N-o-Z Jul 31, 2023

nopcoder Aug 1, 2023

nopcoder Aug 1, 2023

nopcoder Aug 1, 2023

nopcoder Aug 1, 2023

N-o-Z Aug 1, 2023

nopcoder Aug 1, 2023

nopcoder Aug 1, 2023

nopcoder Aug 1, 2023

N-o-Z Aug 1, 2023

nopcoder left a comment

nopcoder Aug 2, 2023

nopcoder Aug 2, 2023

N-o-Z Aug 2, 2023

nopcoder Aug 2, 2023

N-o-Z Aug 2, 2023

lakectl local: implement init #6280

lakectl local: implement init #6280

Conversation

N-o-Z commented Jul 30, 2023 • edited Loading

Linked Issue

Change Description

Background

New Feature

Testing Details

Breaking Change?

github-actions bot commented Jul 30, 2023 • edited Loading

Choose a reason for hiding this comment

N-o-Z Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nopcoder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-o-Z commented Jul 30, 2023 •

edited

Loading

github-actions bot commented Jul 30, 2023 •

edited

Loading

N-o-Z Jul 31, 2023 •

edited

Loading