-
Notifications
You must be signed in to change notification settings - Fork 17
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
Explicitly convert inMediaType json to outMediaType yaml #54
Conversation
Signed-off-by: Deathstroke <[email protected]>
I ran some local tests to see if this works for CRDs, storing data in JSON format, such as
the logic of this PR will still correctly display the rare cases where certain CRDs cannot be decoded from JSON to YAML due to decoding issues. It simply optimizes the user interaction for decoding 'regular CRDs'. Therefore, I think there's no need to do extensive testing. What do you think? @siyuanfoundation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😄 I think we can just use yaml.YAMLToJSON
and yaml.JSONToYAML
from sigs.k8s.io/yaml
I made a similar change in another repo.
https://github.com/kubernetes-sigs/kwok/blob/a284d0ca8703257a9b36a48bb4de328898268162/pkg/kwokctl/etcd/etcd.go#L61-L75
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right about this.
It is fair to say that we do not have to cover all the corner cases. Can you rebase and make sure the tests are green? |
Signed-off-by: Deathstroke <[email protected]>
I forgot that the previous PR changed the number of return values, it has now been fixed. |
/cc @jmhbnz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - Thanks @Deathstr0ke1
Since you mentioned in #33 that CRDs can be read using
--output=json,
I believe this indicates that auger is functionally capable of correctly accessing data in theapplication/json
format (similarly for other data types like CRDs which are also inapplication/json
format). Therefore, I think that when attempting to read this data without the--output=json
parameter, the outputerror decoding from application/json: xxxx
is an unfriendly of program logic, because this data can indeed be accessed normally and should not throw an error.I modified the logic in
encoding.go
so that wheninMediaType
isjson
andoutMediaType
isyaml
(the default case), it explicitly converts thejson
data toyaml
format for output.