-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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 networkMode to AKS 20200201 API #8343
Add networkMode to AKS 20200201 API #8343
Conversation
Azure Pipelines successfully started running 1 pipeline(s). |
1 similar comment
Azure Pipelines successfully started running 1 pipeline(s). |
azure-sdk-for-python
|
azure-sdk-for-net
|
azure-sdk-for-js
|
azure-sdk-for-java
|
azure-sdk-for-go
|
@gtxistxgao FYI |
"networkMode": { | ||
"type": "string", | ||
"enum": [ | ||
"", |
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.
"" [](start = 12, length = 2)
What does empty string mean here?
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.
@majastrz, this means that the networkMode is not set.
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.
Can we use a non-empty value to represent that? Something like none
or default
or 'NotSet`?
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.
Discussed over IM. The API is apparently already released, which violates the usual review workflow (swagger-first)
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.
thank you, will follow that.
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.
This is breaking C# generated code, see https://github.com/AzureSDKAutomation/azure-sdk-for-net/pull/418/files#diff-2f25f07072c1b2f687283e3233db4dc6R19
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.
Signed off from ARM side given that the API is already released. Going forward, you need to get the swagger reviewed first before implementing it in the backend. This way feedback can actually be incorporated into the design of the API.
Thanks, that seems reasonable, we'll do that moving forward. @ArcturusZhang let us know if anything else is needed before merging |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
/azp run automation - sdk |
Azure Pipelines successfully started running 1 pipeline(s). |
@nullMDR can you help check why Python sdk is fail? |
@changlong-liu @ArcturusZhang Could you please help on the autorest failure? |
Well, the root cause is that there is an empty string as enum value, which is breaking both go SDK and C# SDK generated code. |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run automation - sdk |
Azure Pipelines successfully started running 1 pipeline(s). |
4c1552b
into
Azure:dev-containerservice-Microsoft.ContainerService-2020-02-01
Latest improvements:
MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.
Contribution checklist:
ARM API Review Checklist
Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs.
Please follow the link to find more details on API review process.