Skip to content

Commit

Permalink
support prefix parameter in lakectl diff command (#7832)
Browse files Browse the repository at this point in the history
* support prefix parameter in lakectl diff command

* update docs

* write first test

* rename

* use string builder

* add more tests

* rearrange args

* suggest builder pattern for lakectl command in tests (#7836)

* suggest builder pattern for lakectl command in tests

* fix lint

* add Flag api

* use Arg in Flag

* review fixes

* update docs
  • Loading branch information
yonipeleg33 authored Jun 9, 2024
1 parent 237aaac commit 773f2d0
Show file tree
Hide file tree
Showing 5 changed files with 245 additions and 7 deletions.
15 changes: 10 additions & 5 deletions cmd/lakectl/cmd/diff.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ const (
maxDiffPageSize = 1000

twoWayFlagName = "two-way"
prefixFlagName = "prefix"
)

var diffCmd = &cobra.Command{
Expand All @@ -42,7 +43,10 @@ var diffCmd = &cobra.Command{
Uncommitted changes are not shown.
lakectl diff --%s lakefs://example-repo/main lakefs://example-repo/dev$
Show changes between the tip of the main and the dev branch, including uncommitted changes on dev.`, twoWayFlagName, twoWayFlagName),
Show changes between the tip of the main and the dev branch, including uncommitted changes on dev.
lakectl diff --%s some/path lakefs://example-repo/main lakefs://example-repo/dev
Show changes of objects prefixed with 'some/path' between the tips of the main and dev branches.`, twoWayFlagName, twoWayFlagName, prefixFlagName),

Args: cobra.RangeArgs(diffCmdMinArgs, diffCmdMaxArgs),
ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
Expand All @@ -62,13 +66,14 @@ var diffCmd = &cobra.Command{
}

twoWay := Must(cmd.Flags().GetBool(twoWayFlagName))
prefix := Must(cmd.Flags().GetString(prefixFlagName))
leftRefURI := MustParseRefURI("left ref", args[0])
rightRefURI := MustParseRefURI("right ref", args[1])
fmt.Printf("Left ref: %s\nRight ref: %s\n", leftRefURI, rightRefURI)
if leftRefURI.Repository != rightRefURI.Repository {
Die("both references must belong to the same repository", 1)
}
printDiffRefs(cmd.Context(), client, leftRefURI, rightRefURI, twoWay)
printDiffRefs(cmd.Context(), client, leftRefURI, rightRefURI, twoWay, prefix)
},
}

Expand Down Expand Up @@ -109,11 +114,11 @@ func printDiffBranch(ctx context.Context, client apigen.ClientWithResponsesInter
}
}

func printDiffRefs(ctx context.Context, client apigen.ClientWithResponsesInterface, left, right *uri.URI, twoDot bool) {
func printDiffRefs(ctx context.Context, client apigen.ClientWithResponsesInterface, left, right *uri.URI, twoDot bool, prefix string) {
diffs := make(chan apigen.Diff, maxDiffPageSize)
var wg errgroup.Group
wg.Go(func() error {
return diff.StreamRepositoryDiffs(ctx, client, left, right, "", diffs, twoDot)
return diff.StreamRepositoryDiffs(ctx, client, left, right, prefix, diffs, twoDot)
})
for d := range diffs {
FmtDiff(d, true)
Expand All @@ -140,6 +145,6 @@ func FmtDiff(d apigen.Diff, withDirection bool) {
//nolint:gochecknoinits
func init() {
diffCmd.Flags().Bool(twoWayFlagName, false, "Use two-way diff: show difference between the given refs, regardless of a common ancestor.")

diffCmd.Flags().String(prefixFlagName, "", "Show only changes in the given prefix.")
rootCmd.AddCommand(diffCmd)
}
8 changes: 6 additions & 2 deletions docs/reference/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -2039,14 +2039,18 @@ lakectl diff <ref URI> [ref URI] [flags]
lakectl diff --two-way lakefs://example-repo/main lakefs://example-repo/dev$
Show changes between the tip of the main and the dev branch, including uncommitted changes on dev.
lakectl diff --prefix some/path lakefs://example-repo/main lakefs://example-repo/dev
Show changes of objects prefixed with 'some/path' between the tips of the main and dev branches.
```

#### Options
{:.no_toc}

```
-h, --help help for diff
--two-way Use two-way diff: show difference between the given refs, regardless of a common ancestor.
-h, --help help for diff
--prefix string Show only changes in the given prefix.
--two-way Use two-way diff: show difference between the given refs, regardless of a common ancestor.
```


Expand Down
3 changes: 3 additions & 0 deletions esti/golden/lakectl_diff.golden
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Left ref: lakefs://${REPO}/${LEFT_BRANCH}
Right ref: lakefs://${REPO}/${RIGHT_BRANCH}
${DIFF_LIST}
37 changes: 37 additions & 0 deletions esti/lakectl_command.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
package esti

import "strings"

type LakeCtlCmd struct {
rawCmd string
}

func NewLakeCtl() *LakeCtlCmd {
return &LakeCtlCmd{
rawCmd: Lakectl(),
}
}

func (l *LakeCtlCmd) Arg(arg string) *LakeCtlCmd {
l.rawCmd += " " + arg
return l
}

// Flag Same as Arg, added for usage clarity.
func (l *LakeCtlCmd) Flag(arg string) *LakeCtlCmd {
return l.Arg(arg)
}

func (l *LakeCtlCmd) PathArg(components ...string) *LakeCtlCmd {
l.rawCmd += " " + strings.Join(components, "/")
return l
}

func (l *LakeCtlCmd) URLArg(schemaPrefix string, components ...string) *LakeCtlCmd {
l.rawCmd += " " + schemaPrefix + strings.Join(components, "/")
return l
}

func (l *LakeCtlCmd) Get() string {
return l.rawCmd
}
189 changes: 189 additions & 0 deletions esti/lakectl_diff_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,189 @@
package esti

import (
"sort"
"strings"
"testing"
)

const (
filePath1 = "path/to/file1.txt"
filePath2 = "path/to/other/file2.txt"
testBranch = "test"
)

var filesForDiffTest = map[string]string{
filePath1: "ro_1k",
filePath2: "ro_1k_other",
}

func TestLakectlDiffAddedFiles(t *testing.T) {
repoName := generateUniqueRepositoryName()
storage := generateUniqueStorageNamespace(repoName)

createRepo(t, repoName, storage)
createBranch(t, repoName, storage, testBranch)

uploadFiles(t, repoName, testBranch, filesForDiffTest)
commit(t, repoName, testBranch, "adding test files to "+testBranch)

expectedDiff := &ExpectedDiff{
added: []string{filePath1, filePath2},
deleted: []string{},
}
runDiffAndExpect(t, repoName, testBranch, "", expectedDiff)
}

func TestLakectlDiffDeletedFiles(t *testing.T) {
repoName := generateUniqueRepositoryName()
storage := generateUniqueStorageNamespace(repoName)

createRepo(t, repoName, storage)
uploadFiles(t, repoName, mainBranch, filesForDiffTest)
commit(t, repoName, mainBranch, "adding test files to "+mainBranch)

createBranch(t, repoName, storage, testBranch)
deleteFiles(t, repoName, testBranch, filePath1, filePath2)
commit(t, repoName, testBranch, "deleting test files from "+testBranch)

expectedDiff := &ExpectedDiff{
added: []string{},
deleted: []string{filePath1, filePath2},
}
runDiffAndExpect(t, repoName, testBranch, "", expectedDiff)
}

func TestLakectlDiffPrefix(t *testing.T) {
repoName := generateUniqueRepositoryName()
storage := generateUniqueStorageNamespace(repoName)

createRepo(t, repoName, storage)
createBranch(t, repoName, storage, testBranch)

uploadFiles(t, repoName, testBranch, filesForDiffTest)
commit(t, repoName, testBranch, "adding test files to "+testBranch)

runDiffAndExpect(t, repoName, testBranch, "path/to/", &ExpectedDiff{
added: []string{filePath1, filePath2},
deleted: []string{},
})
runDiffAndExpect(t, repoName, testBranch, "path/to/o", &ExpectedDiff{
added: []string{filePath2},
deleted: []string{},
})
runDiffAndExpect(t, repoName, testBranch, "path/to/f", &ExpectedDiff{
added: []string{filePath1},
deleted: []string{},
})
runDiffAndExpect(t, repoName, testBranch, "path/to/x", &ExpectedDiff{
added: []string{},
deleted: []string{},
})
}

type ExpectedDiff struct {
added []string
deleted []string
}

func (a ExpectedDiff) buildAssertionString() string {
type PrefixedFile struct {
prefix string
path string
}
var added []PrefixedFile
var deleted []PrefixedFile
for _, file := range a.added {
added = append(added, PrefixedFile{"+ added ", file})
}
for _, file := range a.deleted {
deleted = append(deleted, PrefixedFile{"- removed ", file})
}
var all = append(added, deleted...)
sort.Slice(all, func(i, j int) bool {
return all[i].path < all[j].path
})

var sb strings.Builder
for _, file := range all {
sb.WriteString(file.prefix)
sb.WriteString(file.path)
sb.WriteString("\n")
}
return sb.String()
}

func runDiffAndExpect(t *testing.T, repoName string, testBranch string, prefix string, diffArgs *ExpectedDiff) {
diffVars := map[string]string{
"REPO": repoName,
"LEFT_BRANCH": mainBranch,
"RIGHT_BRANCH": testBranch,
"DIFF_LIST": diffArgs.buildAssertionString(),
}

cmd := NewLakeCtl().
Arg("diff").
URLArg("lakefs://", repoName, mainBranch).
URLArg("lakefs://", repoName, testBranch)
if prefix != "" {
cmd.Flag("--prefix").Arg(prefix)
}

RunCmdAndVerifySuccessWithFile(t, cmd.Get(), false, "lakectl_diff", diffVars)
}

func uploadFiles(t *testing.T, repoName string, branch string, files map[string]string) {
for filePath, contentPath := range files {
cmd := NewLakeCtl().
Arg("fs upload").
Flag("-s").
PathArg("files", contentPath).
URLArg("lakefs://", repoName, branch, filePath)
RunCmdAndVerifyContainsText(t, cmd.Get(), false, filePath, nil)
}
}

func commit(t *testing.T, repoName string, branch string, commitMessage string) {
cmd := NewLakeCtl().
Arg("commit").
URLArg("lakefs://", repoName, branch).
Flag("-m").
Arg("\"" + commitMessage + "\"")
RunCmdAndVerifyContainsText(t, cmd.Get(), false, commitMessage, nil)
}

func deleteFiles(t *testing.T, repoName string, branch string, files ...string) {
for _, filePath := range files {
cmd := NewLakeCtl().
Arg("fs rm").
URLArg("lakefs://", repoName, branch, filePath)
RunCmdAndVerifySuccess(t, cmd.Get(), false, "", nil)
}
}

func createRepo(t *testing.T, repoName string, storage string) {
cmd := NewLakeCtl().
Arg("repo create").
URLArg("lakefs://", repoName).
Arg(storage)
RunCmdAndVerifySuccessWithFile(t, cmd.Get(), false, "lakectl_repo_create", map[string]string{
"REPO": repoName,
"STORAGE": storage,
"BRANCH": mainBranch,
})
}

func createBranch(t *testing.T, repoName string, storage string, branch string) {
branchVars := map[string]string{
"REPO": repoName,
"STORAGE": storage,
"SOURCE_BRANCH": mainBranch,
"DEST_BRANCH": branch,
}
cmd := NewLakeCtl().
Arg("branch create").
URLArg("lakefs://", repoName, branch).
Flag("--source").
URLArg("lakefs://", repoName, mainBranch)
RunCmdAndVerifySuccessWithFile(t, cmd.Get(), false, "lakectl_branch_create", branchVars)
}

0 comments on commit 773f2d0

Please sign in to comment.