Skip to content

handle empty lists for yaml and json formatted lists in tctl#33547

Merged
stevenGravy merged 15 commits intomasterfrom
stevenGravy/formatlist
Oct 18, 2023
Merged

handle empty lists for yaml and json formatted lists in tctl#33547
stevenGravy merged 15 commits intomasterfrom
stevenGravy/formatlist

Conversation

@stevenGravy
Copy link
Copy Markdown
Contributor

@stevenGravy stevenGravy commented Oct 16, 2023

fixes #33537 and #18672

This adds handling like tsh for giving empty YAML and JSON formatted results. Currently this can return null or a text message of no values.
ex:

$ tctl nodes ls --format=json
null

Now returns

[]

@stevenGravy stevenGravy enabled auto-merge October 16, 2023 20:21
@github-actions github-actions Bot added size/sm tctl tctl - Teleport admin tool labels Oct 16, 2023
@github-actions github-actions Bot requested review from jimbishopp and zmb3 October 16, 2023 20:22
@stevenGravy stevenGravy changed the title handle empty lists for yaml and json lists in tctl handle empty lists for yaml and json formatted lists in tctl Oct 16, 2023
Copy link
Copy Markdown
Collaborator

@zmb3 zmb3 left a comment

Choose a reason for hiding this comment

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

This is a pretty unidiomatic thing to do, normally we prefer the zero value (a nil slice).

Let's move this special case down for each branch so that it is only performed if the serialization format is JSON/YAML.

Comment thread tool/tctl/common/alert_command.go Outdated
Comment thread tool/tctl/common/app_command.go Outdated
Comment thread lib/utils/jsontools.go Outdated
Comment thread lib/utils/jsontools.go Outdated
Comment thread lib/utils/jsontools.go Outdated
Comment thread lib/utils/jsontools.go
Comment thread lib/utils/jsontools.go Outdated
return data, nil
}

// WriteJSON marshals array as a JSON list with indentation.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We should have tests for these that ensure that nil slices/maps get marshaled to [] and {}.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A test for this would be good, since it looks like there is a bug there.

Comment thread tool/tctl/common/bots_command.go Outdated
Comment thread tool/tctl/common/recordings_command.go Outdated
Comment thread tool/tctl/common/token_command.go Outdated
Comment thread tool/tctl/common/token_command.go Outdated
Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com>
Comment thread tool/tctl/common/collection.go Outdated
Comment thread tool/tctl/common/collection.go Outdated
Comment thread tool/tctl/common/token_command.go Outdated
Comment thread tool/tctl/common/loginrule/command.go Outdated
stevenGravy and others added 2 commits October 17, 2023 13:41
Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com>
Copy link
Copy Markdown
Contributor

@camscale camscale left a comment

Choose a reason for hiding this comment

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

One bug I think - the others are a matter of taste - happy for you to ignore those.

I haven't approved because you have auto-merge on and I don't want the bug merged.

Comment thread lib/utils/jsontools.go Outdated
Comment thread lib/utils/jsontools.go
// WriteJSONArray marshals values as a JSON array.
func WriteJSONArray[T any](w io.Writer, values []T) error {
if len(values) == 0 {
_, err := w.Write([]byte("[]"))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Rather that writing a string/bytes directly, perhaps just let json.Marshal do it. null is emitted for nil slices, and [] for empty non-nil slices (see https://go.dev/play/p/MUAis-9xFXn). So perhaps just convert it to a non-nil slice if nil?

if values == nil {
    values = []T{}
}
return writeJSON(w, values)

Comment thread lib/utils/jsontools.go Outdated
// WriteJSONObject marshals m as a JSON object.
func WriteJSONObject[M ~map[K]V, K comparable, V any](w io.Writer, m M) error {
if len(m) == 0 {
_, err := w.Write([]byte("[]"))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same here wrt nil vs empty:

if m == nil {
    m = map[K]V{}
}
return writeJSON(w, m)

Comment thread tool/tctl/common/token_command_test.go Outdated
Comment thread lib/utils/jsontools.go
// first pass makes sure that all values are documents (objects or maps)
slice := reflect.ValueOf(values)
if slice.Len() == 0 {
_, err := w.Write([]byte("[]"))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

there's probably a way to get reflect to produce a non-nil slice, but every time I look at those package docs, I don't come away in less than half an hour :) Probably MakeSlice() but the way you have it is probably simpler in this specific case.

@stevenGravy stevenGravy disabled auto-merge October 18, 2023 01:54
@stevenGravy stevenGravy enabled auto-merge October 18, 2023 02:09
@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from jimbishopp October 18, 2023 02:11
@stevenGravy stevenGravy added this pull request to the merge queue Oct 18, 2023
@stevenGravy stevenGravy removed this pull request from the merge queue due to a manual request Oct 18, 2023
@stevenGravy stevenGravy enabled auto-merge October 18, 2023 02:44
@stevenGravy stevenGravy added this pull request to the merge queue Oct 18, 2023
Merged via the queue into master with commit 4989833 Oct 18, 2023
@stevenGravy stevenGravy deleted the stevenGravy/formatlist branch October 18, 2023 03:24
@public-teleport-github-review-bot
Copy link
Copy Markdown

@stevenGravy See the table below for backport results.

Branch Result
branch/v12 Failed
branch/v13 Failed
branch/v14 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/sm tctl tctl - Teleport admin tool

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tctl users ls on fresh cluster returns null instead of empty array

3 participants