Skip to content

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

guy-har
Copy link
Contributor

@guy-har guy-har commented Jun 10, 2025

No description provided.

Copy link

github-actions bot commented Jun 10, 2025

🎊 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

@guy-har guy-har requested review from nopcoder and arielshaqed June 10, 2025 08:18
@guy-har guy-har added the include-changelog PR description should be included in next release changelog label Jun 10, 2025
@guy-har guy-har changed the title Change branch validations to ref validations in log commits Fix | support all refs in lakeFS log Jun 10, 2025
@@ -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>",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Use: "log <Ref URI>",
Use: "log <ref URI>",

Short: "Show log of commits",
Long: "Show log of commits for a given branch",
Long: "Show log of commits for a given Ref",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Long: "Show log of commits for a given Ref",
Long: "Show log of commits for a given a ref",

@@ -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])
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
RefURI := MustParseRefURI("Ref URI", args[0])
refURI := MustParseRefURI("ref URI", args[0])

@@ -141,14 +141,14 @@ var logCmd = &cobra.Command{

graph := &dotWriter{
w: os.Stdout,
repositoryID: branchURI.Repository,
repositoryID: RefURI.Repository,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
repositoryID: RefURI.Repository,
repositoryID: refURI.Repository,

}
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
resp, err := client.LogCommitsWithResponse(cmd.Context(), RefURI.Repository, RefURI.Ref, logCommitsParams)
resp, err := client.LogCommitsWithResponse(cmd.Context(), refURI.Repository, refURI.Ref, logCommitsParams)

@guy-har guy-har requested a review from nopcoder June 10, 2025 14:21
@guy-har guy-har force-pushed the fix/lakectl-log-arg branch from 35c24bb to 2ba8f97 Compare June 10, 2025 14:26
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!

@nopcoder nopcoder added the area/lakectl Issues related to lakeFS' command line interface (lakectl) label Jun 10, 2025
Copy link
Contributor

@arielshaqed arielshaqed left a 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,
},
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: whitespace changes.

@nopcoder nopcoder changed the title Fix | support all refs in lakeFS log Fix: support all refs in lakeFS log Jun 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/lakectl Issues related to lakeFS' command line interface (lakectl) include-changelog PR description should be included in next release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lakectl | lakectl log command should support refs in addition to branches (like lakectl fs ls)
3 participants