2898 lakectl annotate output has superfluous spaces and blank lines #3007

itaidavid · 2022-03-03T15:03:43Z

lakectl annotate output:
Removed line break at the beginning of output line
Removed padding from commit message field

…aces

nopcoder · 2022-03-05T16:47:55Z

nessie/lakectl_util.go

@@ -122,6 +122,7 @@ func sanitize(output string, vars map[string]string) string {
 	s = normalizeRandomObjectKey(s, vars["STORAGE"])
 	s = normalizeCommitID(s)
 	s = normalizeChecksum(s)
+	s = normalizeShortCommitID(s)
 	return strings.ReplaceAll(s, "\r\n", "\n")


general note - if we do this one first, there is no need to keep the regexp patterns \r?\n compatible

@nopcoder - not sure I understand this one. Can you please specify what is 'this' in '... this one first'?

replace new lines before running all the regexp patterns

nopcoder · 2022-03-05T17:46:57Z

nessie/lakectl_test.go

+	}
+
+	// create fresh repo with 'main' branch
+	RunCmdAndVerifySuccessWithFile(t, Lakectl()+" repo create lakefs://"+repoName+" "+storage, false, "lakectl_repo_create", vars)


Suggestion to use the table driven format - it will structure the following code and will report each one as a test:

tests := []struct { Name string Cmd string Golden string Vars map[string]string }{ { Name: "log not found", Cmd: "log lakefs://" + repoName + "/" + mainBranch, Golden: "lakectl_log_404", Vars: map[string]string{"REPO": repoName, "STORAGE": storage, "BRANCH": mainBranch}, }, } for _, tt := range tests { t.Run(tt.Name, func(t *testing.T) { RunCmdAndVerifyFailureWithFile(t, Lakectl()+" "+tt.Cmd, false, tt.Golden, vars) }) }

@nopcoder - I like the idea, but I think it is too large of a change to be included with this bug fix.
How about I create an issue for that?

Opened #3021 to handle this

nopcoder · 2022-03-05T17:57:07Z

nessie/lakectl_util.go

@@ -199,6 +201,11 @@ func normalizeCommitID(output string) string {
 	return string(s)
 }

+func normalizeShortCommitID(output string) string {
+	s := shortCommitIDRegExp.ReplaceAll([]byte(output), []byte("<COMMIT_ID_16>"))


Relevant for other places, Go supply the 'string' variant - use: ReplaceAllString

Same as for the previous one - Let's have it as a different PR, along with the refactoring to table-driven. Something like 'Lakectl Test Refactoring'. WDYT?

Jonathan-Rosenberg · 2022-03-09T14:31:50Z

cmd/lakectl/cmd/annotate.go

 		client := getClient()
 		pfx := api.PaginationPrefix(*pathURI.Path)
 		context := cmd.Context()
 		resp, err := client.ListObjectsWithResponse(context, pathURI.Repository, pathURI.Ref, &api.ListObjectsParams{Prefix: &pfx})
 		DieOnErrorOrUnexpectedStatusCode(resp, err, http.StatusOK)
+		const delimiter = "/"


Is this necessary?

Not really necessary, but it is also used the same way in other commands (lakectl fs)
I refactored it to be a const in common_helpers

Jonathan-Rosenberg · 2022-03-09T14:37:44Z

cmd/lakectl/cmd/annotate.go

@@ -29,23 +28,36 @@ var annotateCmd = &cobra.Command{
 	Args:    cobra.ExactArgs(1),
 	Run: func(cmd *cobra.Command, args []string) {
 		pathURI := MustParsePathURI("path", args[0])
+		recursive, _ := cmd.Flags().GetBool("recursive")


can make "recursive" const as being used for command registration (line 97) and value retrieval.

It will "break" the way we register other flags, including "recursive" in lakectl fs commands. If we change it in all places it interfere with the readability of the commands regsitration, so I think it is better to leave it like that.
@Jonathan-Rosenberg - WDYT?

Interesting... I did see that we use string literals and not variables when defining flags and retrieving their values, but is it a good practice?
Anyway it's not a deal breaker so we can leave it that way.

Jonathan-Rosenberg

Noice

Co-authored-by: Idan Novogroder <43949240+idanovo@users.noreply.github.com>

Co-authored-by: talSofer <tal.sofer@treeverse.io>

Co-authored-by: shimi9276 <71839784+shimi9276@users.noreply.github.com>

nopcoder

lgtm

nopcoder · 2022-03-10T09:19:52Z

cmd/lakectl/cmd/common_helpers.go

@@ -32,6 +32,11 @@ var ErrInvalidValueInList = errors.New("empty string in list")

 var accessKeyRegexp = regexp.MustCompile(`^AKIA[I|J][A-Z0-9]{14}Q$`)

+// const values used by lakectl commands


Can remove this one - we are in common_helpers under lakectl

nopcoder · 2022-03-10T09:20:40Z

cmd/lakectl/cmd/common_helpers.go

@@ -32,6 +32,11 @@ var ErrInvalidValueInList = errors.New("empty string in list")

 var accessKeyRegexp = regexp.MustCompile(`^AKIA[I|J][A-Z0-9]{14}Q$`)

+// const values used by lakectl commands
+const (


general note - not actionable to this pr - but it looks like we have a lot of 'const' blocks here

…us-spaces-and-blank-lines

Itai David added 2 commits March 2, 2022 22:18

lakectl annotate should default to non recursive listing

37ac145

purifying lakectl annotate output from redundant newlines and whitesp…

770a831

…aces

itaidavid added the include-changelog PR description should be included in next release changelog label Mar 3, 2022

itaidavid linked an issue Mar 3, 2022 that may be closed by this pull request

BUG: lakectl annotate output has superfluous spaces and blank lines #2898

Closed

nopcoder requested changes Mar 5, 2022

View reviewed changes

code review - lakectl test util improvements

ea1295d

itaidavid mentioned this pull request Mar 9, 2022

refactor lakectl test - use table driven format for tests #3021

Closed

itaidavid requested a review from nopcoder March 9, 2022 04:03

Jonathan-Rosenberg reviewed Mar 9, 2022

View reviewed changes

Jonathan-Rosenberg approved these changes Mar 9, 2022

View reviewed changes

itaidavid and others added 5 commits March 9, 2022 12:46

Update cmd/lakectl/cmd/annotate.go

199829e

Co-authored-by: Idan Novogroder <43949240+idanovo@users.noreply.github.com>

Update cmd/lakectl/cmd/annotate.go

cd9fa1e

Co-authored-by: talSofer <tal.sofer@treeverse.io>

Update cmd/lakectl/cmd/annotate.go

0468415

Co-authored-by: shimi9276 <71839784+shimi9276@users.noreply.github.com>

implementing review comments

74bc055

refactoring path delimiter to common_helpers

7efc189

nopcoder approved these changes Mar 10, 2022

View reviewed changes

itaidavid and others added 2 commits March 17, 2022 11:54

Merge branch 'master' into 2898-lakectl-annotate-output-has-superfluo…

2ab6565

…us-spaces-and-blank-lines

Fixing merge errors

3613d52

itaidavid merged commit 2c8c7aa into master Mar 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2898 lakectl annotate output has superfluous spaces and blank lines #3007

2898 lakectl annotate output has superfluous spaces and blank lines #3007

itaidavid commented Mar 3, 2022

nopcoder Mar 5, 2022

itaidavid Mar 7, 2022

nopcoder Mar 8, 2022

itaidavid Mar 9, 2022

nopcoder Mar 5, 2022

itaidavid Mar 7, 2022

itaidavid Mar 9, 2022

nopcoder Mar 5, 2022

itaidavid Mar 7, 2022

itaidavid Mar 9, 2022

Jonathan-Rosenberg Mar 9, 2022

itaidavid Mar 9, 2022 •

edited

Loading

Jonathan-Rosenberg Mar 9, 2022

itaidavid Mar 9, 2022

Jonathan-Rosenberg Mar 10, 2022 •

edited

Loading

Jonathan-Rosenberg left a comment

nopcoder left a comment

nopcoder Mar 10, 2022

nopcoder Mar 10, 2022

		@@ -32,6 +32,11 @@ var ErrInvalidValueInList = errors.New("empty string in list")

		var accessKeyRegexp = regexp.MustCompile(`^AKIA[I\|J][A-Z0-9]{14}Q$`)

		// const values used by lakectl commands

2898 lakectl annotate output has superfluous spaces and blank lines #3007

2898 lakectl annotate output has superfluous spaces and blank lines #3007

Conversation

itaidavid commented Mar 3, 2022

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

itaidavid Mar 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jonathan-Rosenberg Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

Jonathan-Rosenberg left a 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

itaidavid Mar 9, 2022 •

edited

Loading

Jonathan-Rosenberg Mar 10, 2022 •

edited

Loading