Skip to content
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

lakectl log: add option to filter merge commits #8142

Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion cmd/lakectl/cmd/common_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,10 @@ const (
internalPageSize = 1000 // when retrieving all records, use this page size under the hood
defaultAmountArgumentValue = 100 // when no amount is specified, use this value for the argument

// when using --no-merges & amount, this const is the upper limit to use huristic.
// when using --no-merges & amount, this const is the upper limit to use heuristic.
// The heuristic asks for 3*amount results since some will filter out
maxAmountNoMerges = 333
noMergesHeuristic = 3

defaultPollInterval = 3 * time.Second // default interval while pulling tasks status
minimumPollInterval = time.Second // minimum interval while pulling tasks status
Expand Down
4 changes: 2 additions & 2 deletions cmd/lakectl/cmd/log.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ var logCmd = &cobra.Command{
}
// case --no-merges & --amount, ask for more results since some will filter out
if noMerges && amountForPagination < maxAmountNoMerges {
amountForPagination *= 3
amountForPagination *= noMergesHeuristic
}
logCommitsParams := &apigen.LogCommitsParams{
After: apiutil.Ptr(apigen.PaginationAfter(after)),
Expand Down Expand Up @@ -172,8 +172,8 @@ var logCmd = &cobra.Command{
// case --no-merges, filter commits and subtract that amount from amount desired
if noMerges {
data.Commits = filterMergeCommits(data.Commits)
amount -= len(data.Commits)
}
amount -= len(data.Commits)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this still prints an incorrect number of commits. Think of the case where you get more commits after filtering than you really needed. Something like data.Commits = data.Commits[:amount] should do the job.

Please also add a test that would fail without this fix.


if dot {
graph.Write(data.Commits)
Expand Down
1 change: 0 additions & 1 deletion esti/lakectl_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,6 @@ func TestLakectlMergeAndStrategies(t *testing.T) {
}

func TestLakectlLogNoMergesWithCommitsAndMerges(t *testing.T) {

repoName := generateUniqueRepositoryName()
storage := generateUniqueStorageNamespace(repoName)
vars := map[string]string{
Expand Down
Loading