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

Explicitly convert inMediaType json to outMediaType yaml #54

Merged
merged 2 commits into from
May 19, 2024

Conversation

Deathstr0ke1
Copy link
Contributor

CRDs currently can be read with --output=json and that is generally how it is encoded in k8s as well. I think there are discussions regarding other encoders for CRDs. Until that changes, we do not really need to worry about that. However we do need to add better documentation of how to decode CRDs.

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 the application/json format (similarly for other data types like CRDs which are also in application/json format). Therefore, I think that when attempting to read this data without the --output=json parameter, the output error 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 when inMediaType is json and outMediaType is yaml(the default case), it explicitly converts the json data to yaml format for output.

Signed-off-by: Deathstroke <776022703@qq.com>
@Deathstr0ke1
Copy link
Contributor Author

Deathstr0ke1 commented May 11, 2024

I ran some local tests to see if this works for CRDs, storing data in JSON format, such as /registry/apiregistration.k8s.io/apiservices/v1., /registry/cilium.io/ciliumidentities/xxx.
BTW, I believe that since under the original logic:

try to decode any CRDs storing data in JSON format in default case output=yaml would throw errors

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

Copy link
Member

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

Copy link
Contributor Author

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.

@siyuanfoundation
Copy link
Contributor

I ran some local tests to see if this works for CRDs, storing data in JSON format, such as /registry/apiregistration.k8s.io/apiservices/v1., /registry/cilium.io/ciliumidentities/xxx. BTW, I believe that since under the original logic:

try to decode any CRDs storing data in JSON format in default case output=yaml would throw errors

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

It is fair to say that we do not have to cover all the corner cases.
We do need to add more unit tests for this package. But that can be a followup.

Can you rebase and make sure the tests are green?

Signed-off-by: Deathstroke <776022703@qq.com>
@Deathstr0ke1
Copy link
Contributor Author

I forgot that the previous PR changed the number of return values, it has now been fixed.

@siyuanfoundation
Copy link
Contributor

/cc @jmhbnz

@k8s-ci-robot k8s-ci-robot requested a review from jmhbnz May 15, 2024 23:55
Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

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

LGTM - Thanks @Deathstr0ke1

@jmhbnz jmhbnz merged commit 4c462f6 into etcd-io:master May 19, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants