Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

kubeConfig should have "format": "byte" along with "type": "string" #2546

Closed
amarzavery opened this issue Feb 27, 2018 · 5 comments · Fixed by #2669
Closed

kubeConfig should have "format": "byte" along with "type": "string" #2546

amarzavery opened this issue Feb 27, 2018 · 5 comments · Fixed by #2669
Assignees

Comments

@amarzavery
Copy link
Contributor

amarzavery commented Feb 27, 2018

If "format": "byte" is specified then the generated code will be able to serialize and deserialize this property kubeconfig correctly.

As per swagger specification, this is the recommended way of describing base64 encoded characters.

@mboersma - Please make sure to fix this.
/cc @milismsft

@amarzavery
Copy link
Contributor Author

/cc @lmazuel - FYI.

@lmazuel
Copy link
Member

lmazuel commented Feb 27, 2018

@mboersma I was looking at CLI, and it seems this will just change this:

        encoded_kubeconfig = access_profile.kube_config
        kubeconfig = base64.b64decode(encoded_kubeconfig).decode(encoding='UTF-8')

to this:

        kubeconfig = access_profile.kube_config.decode(encoding='UTF-8')

while at the same time providing free base64 decoding for SDK customers

amarzavery added a commit to amarzavery/azure-sdk-for-node that referenced this issue Feb 27, 2018
…han adding a convenience wrapper, I am modifying the generated code to do the right thing. Related issue: Azure/azure-rest-api-specs#2546
@milismsft
Copy link
Contributor

FYI I would prefer if this change is done as part of a new API version rather than using the same version as it was done not to far in the past for other "non-breaking" changes (see access profiles)... Especially since it's not blocking and can be easily worked around in the SDKs for now.

@mboersma
Copy link
Member

This does seem like an obvious improvement.

@milismsft I've encountered a lot of resistance when proposing we rev the AKS API version before, and in this case we're not even changing server-side code. If it's possible to inline such a change into v20170831, that may be the only way to accomplish this in a reasonable time frame.

@amarzavery
Copy link
Contributor Author

@mboersma - I have already made this change in the node sdk over here and published the sdk. This was easier than adding a layer on top that would do the right thing.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants