-
Notifications
You must be signed in to change notification settings - Fork 380
Fix: support all refs in lakeFS log #9170
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
base: master
Are you sure you want to change the base?
Conversation
🎊 PR Preview 2ba8f97 has been successfully built and deployed to https://treeverse-lakeFS-preview-pr-9170.surge.sh 🕐 Build time: 0.012s 🤖 By surge-preview |
cmd/lakectl/cmd/log.go
Outdated
@@ -81,9 +81,9 @@ func filterMergeCommits(commits []apigen.Commit) []apigen.Commit { | |||
|
|||
// logCmd represents the log command | |||
var logCmd = &cobra.Command{ | |||
Use: "log <branch URI>", | |||
Use: "log <Ref URI>", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use: "log <Ref URI>", | |
Use: "log <ref URI>", |
cmd/lakectl/cmd/log.go
Outdated
Short: "Show log of commits", | ||
Long: "Show log of commits for a given branch", | ||
Long: "Show log of commits for a given Ref", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Long: "Show log of commits for a given Ref", | |
Long: "Show log of commits for a given a ref", |
cmd/lakectl/cmd/log.go
Outdated
@@ -109,7 +109,7 @@ var logCmd = &cobra.Command{ | |||
pagination := apigen.Pagination{HasMore: true} | |||
showMetaRangeID := Must(cmd.Flags().GetBool("show-meta-range-id")) | |||
client := getClient() | |||
branchURI := MustParseBranchURI("branch URI", args[0]) | |||
RefURI := MustParseRefURI("Ref URI", args[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RefURI := MustParseRefURI("Ref URI", args[0]) | |
refURI := MustParseRefURI("ref URI", args[0]) |
cmd/lakectl/cmd/log.go
Outdated
@@ -141,14 +141,14 @@ var logCmd = &cobra.Command{ | |||
|
|||
graph := &dotWriter{ | |||
w: os.Stdout, | |||
repositoryID: branchURI.Repository, | |||
repositoryID: RefURI.Repository, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
repositoryID: RefURI.Repository, | |
repositoryID: refURI.Repository, |
cmd/lakectl/cmd/log.go
Outdated
} | ||
if dot { | ||
graph.Start() | ||
} | ||
|
||
for pagination.HasMore { | ||
resp, err := client.LogCommitsWithResponse(cmd.Context(), branchURI.Repository, branchURI.Ref, logCommitsParams) | ||
resp, err := client.LogCommitsWithResponse(cmd.Context(), RefURI.Repository, RefURI.Ref, logCommitsParams) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resp, err := client.LogCommitsWithResponse(cmd.Context(), RefURI.Repository, RefURI.Ref, logCommitsParams) | |
resp, err := client.LogCommitsWithResponse(cmd.Context(), refURI.Repository, refURI.Ref, logCommitsParams) |
35c24bb
to
2ba8f97
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat, thanks!
In general I am against almost any client-side validation: it merely reduces usability, for instance old clients might fail with new servers for no good reason.
@@ -168,7 +168,6 @@ var logCmd = &cobra.Command{ | |||
After: pagination.NextOffset, | |||
}, | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: whitespace changes.
No description provided.