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

fix: print consistent json/yaml output #1094

Merged
merged 1 commit into from
Jul 11, 2022

Conversation

devholic
Copy link
Contributor

@devholic devholic commented Jun 24, 2022

What

Print consistent output for commands.

Why

Currently, some commands (e.g. k3d cluster list or k3d cluster get) prints differently per output option (json / yaml). This behavior may break interoperability and can make users hard to figure out the difference between outputs.

Changes

Replace gopkg.in/yaml.v2 with sigs.k8s.io/yaml, which converts the json-encoded struct to yaml.

(sigs.k8s.io/yaml is a wrapper for gopkg.in/yaml.v2 - https://github.com/kubernetes-sigs/yaml/blob/b5bdf49df8aeb9756eee686adc7b4a6b3640bc14/yaml.go#L30-L38)

Implications

Commands will print same struct for both json / yaml output options.

This change may breaks previous behavior, since this commit removes yaml tag from structs.

Open Questions

I think it would be better to use either gopkg.in/yaml.v2 or sigs.k8s.io/yaml only across the repository for consistency, and there is an advantage that we don't have to add yaml tags to struct fields. WDYT?

@iwilltry42
Copy link
Member

Hi @devholic , thanks for opening this PR! 👍
I agree with your observation and it sounds like a good idea to consistently use sigs.k8s.io/yaml all over the repo.
Would you mind extending your PR to cover this? :)

@iwilltry42 iwilltry42 added this to the v5.5.0 milestone Jun 30, 2022
@devholic
Copy link
Contributor Author

Sure 🙂

devholic added a commit to devholic/k3d that referenced this pull request Jul 1, 2022
@devholic devholic changed the title fix: print consistent output for list clusters fix: print consistent json/yaml output Jul 1, 2022
@devholic devholic force-pushed the cluster-list-consistent-output branch from b11130a to 3099004 Compare July 1, 2022 07:10
@devholic
Copy link
Contributor Author

devholic commented Jul 1, 2022

@iwilltry42 I squashed commit, but you can check changelogs at devholic/k3d@main...devholic:k3d:cluster-list-consistent-output-patch 🙂

@iwilltry42 iwilltry42 self-requested a review July 1, 2022 11:43
Copy link
Member

@iwilltry42 iwilltry42 left a comment

Choose a reason for hiding this comment

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

LGTM

@iwilltry42 iwilltry42 self-requested a review July 1, 2022 11:48
@iwilltry42
Copy link
Member

Thank you very much!
While code-wise it looked good to me, it seems to break cluster creation during tests:
ERRO[2022-07-01T11:47:23Z] Failed Cluster Start: error during post-start cluster preparation: error while rewriting /var/lib/rancher/k3s/server/manifests/coredns.yaml in k3d-registrytest-server-0: error splitting yaml: error marshaling into JSON: json: unsupported type: map[interface {}]interface {}

Currently, some commands (e.g. `k3d cluster list` or `k3d cluster get`)
prints differently per output option (`json` / `yaml`). This behavior
may break interoperability and can make users hard to figure out the
difference between outputs.

This commit fixes this issue by replacing `gopkg.in/yaml.v2` with
`sigs.k8s.io/yaml`, which converts the json-encoded struct to yaml.

Signed-off-by: Sunghoon Kang <hoon@akuity.io>
@devholic devholic force-pushed the cluster-list-consistent-output branch from 3099004 to aac3455 Compare July 1, 2022 17:41
@devholic
Copy link
Contributor Author

devholic commented Jul 1, 2022

@iwilltry42

Thank you very much!
While code-wise it looked good to me, it seems to break cluster creation during tests:

I found that gopkg.in/yaml.v2 uses map[interface{}]interface{} internally, so it breaks SplitYAML and callers. Here is the patch for this issue (squashed) 🙏

devholic/k3d@b11130a...devholic:k3d:cluster-list-consistent-output-patch

@iwilltry42 iwilltry42 self-assigned this Jul 11, 2022
@iwilltry42 iwilltry42 added bug Something isn't working enhancement New feature or request component/main go Pull requests that update Go code labels Jul 11, 2022
Copy link
Member

@iwilltry42 iwilltry42 left a comment

Choose a reason for hiding this comment

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

LGTM! Tests passed ✔️

@iwilltry42
Copy link
Member

Sorry for the late review! Thanks for this addition :)

@iwilltry42
Copy link
Member

@all-contributors please add @devholic for code

@allcontributors
Copy link
Contributor

@iwilltry42

I've put up a pull request to add @devholic! 🎉

@iwilltry42 iwilltry42 merged commit 24bb5f4 into k3d-io:main Jul 11, 2022
@devholic devholic deleted the cluster-list-consistent-output branch July 11, 2022 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working component/main enhancement New feature or request go Pull requests that update Go code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants