Repository navigation
Share stdout/stderr output utils between lakefs and lakectl - #10609
Closed
shaktimaan00 wants to merge 1 commit into
Closed
shaktimaan00 wants to merge 1 commit into
shaktimaan00 wants to merge 1 commit into
Conversation
lakectl had template based helpers for writing to stdout and stderr that could not be imported by the lakefs server binary, so lakefs grew its own ad-hoc print helpers (added for the flare command). Move the shared implementation into pkg/cmdutils/output.go: * ColoredText, Table and Pagination, plus the template functions behind red/yellow/green/blue/bold/underline, json, table, paginate and friends. * WriteTo, Write, Warning, Die, DieFmt, DieErr and PrintTable, along with the death and warning message templates. * InfoFmt and NonFatalErrorFmt, which replace lakefs' printInfo and printNonFatalError. lakectl keeps thin wrappers with its existing signatures, so none of the commands had to change, and lakefs consumes the package directly.
Author
|
Closing this in favour of #10610, which I opened a minute later — same issue, same approach, so having both open just splits the review. Sorry for the duplicate. The two differences, so the choice is clear if you would rather have this one:
Both branches build and their tests pass; nothing in this branch is needed by #10610. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #7904
Change Description
Background
lakectlhas template based helpers for writing to stdout and stderr, but theylive in
cmd/lakectl/cmd/common_helpers.goand cannot be imported by thelakefsserver binary. As a result,lakefsgrew its own small set of printhelpers (
printMsgAndExit,printInfo,printNonFatalError, added for theflarecommand). This moves the shared implementation into a package that bothbinaries import.
New Feature
New package
pkg/cmdutils/output.go(next to the existingcmdutilsprogress/validation/coalesce helpers) with the output utilities:
ColoredText,Table,Paginationand the template functions behindred/yellow/green/blue/bold/underline,date,ljust,json,table,paginate,lower,human_bytesandjoin.WriteTo,Write,Warning,Die,DieFmt,DieErr,PrintTable, plusthe
DeathMessage,DeathMessageWithFields,WarnMessage,InfoMessageandErrorMessagetemplates.DisableColors/IsTerminal, so the "no colors, no terminal only niceties"switch applies to every consumer.
InfoFmtandNonFatalErrorFmt, replacinglakefs'printInfoandprintNonFatalError.cmd/lakectl/cmd/common_helpers.gokeeps thin wrappers with its existingsignatures (
WriteTo(tpl, data, w),Colored,Table/Pagination/ColoredTexttype aliases, ...) so none of the ~135 lakectl command files had to change.
cmd/lakefs/cmd/common_helpers.gois deleted andgc.go/flare.gocallcmdutilsdirectly.Testing Details
go build ./...go vet ./cmd/lakectl/... ./cmd/lakefs/... ./pkg/cmdutils/...go test ./cmd/lakectl/cmd/...— passes, includingTestColors, whichexercises the moved
WriteTotemplate functions through the wrapper.Breaking Change?
No API or client change. One user visible difference: errors from the
lakefsbinary now print through the shared
DeathMessagetemplate, i.e.instead of
Error: <error>.Info/Errornon-fatal messages and the flarewarnings keep their previous meaning, now covering both binaries.