-
Notifications
You must be signed in to change notification settings - Fork 5.6k
Add description clarifying the encoding requirements for KeyOperationsParameters.Value #3286
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
Add description clarifying the encoding requirements for KeyOperationsParameters.Value #3286
Conversation
Automation for azure-sdk-for-pythonA PR has been created for you based on this PR content. Once this PR will be merged, content will be added to your service PR: |
Automation for azure-sdk-for-nodeA PR has been created for you based on this PR content. Once this PR will be merged, content will be added to your service PR: |
Automation for azure-sdk-for-javaNothing to generate for azure-sdk-for-java |
Automation for azure-sdk-for-rubyNothing to generate for azure-sdk-for-ruby |
Automation for azure-sdk-for-goA PR has been created for you: |
|
@schaabs does this description look ok to you? |
|
@carlpett Thanks for the contribution, and I'm sorry for the lack of clarity in this case. The problem with adding this description here is that it will end up in the documentation for all our generated SDKs. Unfortunately there seems to be a discrepancy in the code that gets generated for base64url formated properties in Go vs other languages. Most of the SDK runtimes (Python, Node, C#, Java) handle the base64url encoding during serialization and deserialization and this property is exposed as a byte[]. So in those cases describing the property as a base64url encoded string would be confusing. I think the best course of action is your suggestion to improve the Go code generator. It should either handle the encoding as is done in other languages, or at least document the expected format of the property in the generated code. I've started a conversation with the Go autorest developers, and will follow back up once I have an issue to track. |
|
@schaabs this is a great point, in fact we haven't implemented support for Base64Url primary types in the Go generator, see the comment here. I've opened issue Azure/azure-sdk-for-go#2111 to track the implementation however this will result in a breaking change and we're trying to avoid introducing breaking changes due to changes in the codegen so I don't know when this will actually happen. |
|
@jhendrixMSFT is it possible to update the generator to add docstrings to these properties in the meantime indicating that they should be base64url encoded? |
|
@schaabs I was just thinking that, should be doable. |
|
I've opened PR Azure/autorest.go#137 in the code generator to add this documentation and will get the SDK content updated with it. |
|
@jhendrixMSFT Thanks, that fix should help people avoid the issue until there is support, at least :) |
Moved change from the Go SDK: Azure/azure-sdk-for-go#2061
It seems the swagger doc is clearer than the generated Go comment, with
"format": "base64url", so an option to this PR would be to improve the Go generator, I guess.