-
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
VMware by Virtustream API 2019-08-09-preview #8296
Conversation
You don't have permission to trigger SDK Automation. |
I do not not understand why the LintDiff test is failing. Any help to fix that would be appreciated. |
@ruowan Hello. Could you help about lintdiff? The error message is confusing.
|
@ctaggart Looks like there are some characters which cannot be parsed. Could you check it in detail? |
I'm not sure what the error means, which is why I'm asking for help. I don't see any problems with the encoding of the vmwarevirtustream.json file. May be something is wrong in the readme.md.
I hope to be able to reproduce locally in a Docker container by running |
I am unable to reproduce the problem locally. |
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.
Lots of descriptions are missing. Could we add them? We cannot assume that customers know everything.
It's quite hard to guess the meaning of each field.
}, | ||
{ | ||
"name": "Clusters", | ||
"description": "" |
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 you add corresponding description?
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.
description would be used in SDKs/CLIs as the help message. So it's important to have description.
...source-manager/Microsoft.VMwareVirtustream/preview/2019-08-09-preview/vmwarevirtustream.json
Show resolved
Hide resolved
...source-manager/Microsoft.VMwareVirtustream/preview/2019-08-09-preview/vmwarevirtustream.json
Outdated
Show resolved
Hide resolved
...source-manager/Microsoft.VMwareVirtustream/preview/2019-08-09-preview/vmwarevirtustream.json
Show resolved
Hide resolved
"nsxtPassword": { | ||
"description": "Optionally, set the NSX-T Manager password when the private cloud is created", | ||
"type": "string" | ||
} |
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.
Any password or secret property should be annotated with x-ms-secret: true. (Here and anywhere else.)
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, I don't see any specs using x-ms-secret
. There are several using "secret": true
and I also see "isSecret": true
. Should I be using one of those?
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.
x-ms-secret
was added recently and is not yet documented. Take a look at this query for examples: https://github.com/Azure/azure-rest-api-specs/search?q=x-ms-secret&unscoped_q=x-ms-secret
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.
Thanks. I added "x-ms-secret": true
.
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.
Adding this leads to validation errors of:
Secret property `"nsxtPassword": "$(1X4Dkk"`, cannot be sent in the response.
Let'd put these changes in API 2020-03-20 as well. I'm going to revert the change.
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.
@myronfanqiu this looks like a linter rule bug. Sharing definitions between GET and PUT is one of the accepted patterns and should not block usage of x-ms-secret: true. @ctaggart I'm ok either way, but you should be able to put it back in with a linter rule suppression as a workaround.
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.
Let's put in in API 2020-03-20.
...source-manager/Microsoft.VMwareVirtustream/preview/2019-08-09-preview/vmwarevirtustream.json
Show resolved
Hide resolved
I added some questions and comments. Please take a look. |
Thanks for the review and feedback @majastrz. I've responded to each item. |
@ctaggart I replied back. |
This reverts commit 3358dff.
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.
Thanks @majastrz. What are the next steps? When can this get merged and SDKs built for python, typescript, and csharp? |
@ctaggart, the next step is for the reviewer assigned from the SDK team to sign off on the PR (for this one it's @myronfanqiu). Once signed off, you can ask @myronfanqiu to merge the PR. Your backend should be up and running in PROD when the Swagger changes are merged, however. |
azure-arm: true | ||
package-name: "@azure/arm-vmwarevirtustream" | ||
output-folder: "$(typescript-sdks-folder)/sdk/vmwarevirtustream/arm-vmwarevirtustream" | ||
payload-flattening-threshold: 1 |
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.
Hello. I'm not sure about this value. In other SDKs, you set it to 2. But we can merge it first and left SDK owners to fix it.
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.
add @qiaozha (js sdk owner) for have a double check for this
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.
its okay to set this value as 2 for now. but it's better to have clear-output-folder: true
. we can also add this later.
@ctaggart Hello. I cannot force merge this PR. It's better to fix CI error in this PR. If you insist on merging this one without fix, @akning-ms can help you. |
@akning-ms said:
@ruowan, can you help me fix the LintDiff CI problem? @myronfanqiu says it needs to be fixed in order to merge this. |
azure-sdk-for-net - Release
|
azure-sdk-for-go - Release
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
azure-sdk-for-java - Release
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
azure-sdk-for-js - Release
|
azure-sdk-for-python - Release
|
* 2019-08-09-preview * spell out networks * username and credential needed to add an IdentitySource * post listAdminCredentials * standardize on username, password, nsxt and vcenter * add nsxt to custom-words.txt * default cluster does not have provisioningState * it extends DefaultClusterProperties * add Circuit expressRoutePrivatePeeringID * back out some minor changes for sdk generation * identitySources and Circuit authorizations are updateable * add PrivateClouds_ListInSubscription * Long running operation for Clusters_Delete (Azure#1) * npm run prettier * location for all resources (Azure#2) * Revert "location for all resources (Azure#2)" (Azure#3) This reverts commit 432e37b. * add description TODOs * example is PrivateClouds_Update * require body in PrivateClouds_Update * replaced TODOs with descriptions * spellcheck * "x-ms-secret": true * Revert ""x-ms-secret": true" This reverts commit 3358dff. Co-authored-by: Andrew Ulliani <aulliani@yahoo.com> Co-authored-by: Eugene Tolmachev <e.tolmachev@gmail.com>
* 2019-08-09-preview * spell out networks * username and credential needed to add an IdentitySource * post listAdminCredentials * standardize on username, password, nsxt and vcenter * add nsxt to custom-words.txt * default cluster does not have provisioningState * it extends DefaultClusterProperties * add Circuit expressRoutePrivatePeeringID * back out some minor changes for sdk generation * identitySources and Circuit authorizations are updateable * add PrivateClouds_ListInSubscription * Long running operation for Clusters_Delete (#1) * npm run prettier * location for all resources (#2) * Revert "location for all resources (#2)" (Azure#3) This reverts commit 432e37b. * add description TODOs * example is PrivateClouds_Update * require body in PrivateClouds_Update * replaced TODOs with descriptions * spellcheck * "x-ms-secret": true * Revert ""x-ms-secret": true" This reverts commit 3358dff. Co-authored-by: Andrew Ulliani <aulliani@yahoo.com> Co-authored-by: Eugene Tolmachev <e.tolmachev@gmail.com>
We are in private preview as of Friday, January 31st, so we can finally merge this. It was merged by @yungezz on September 22nd #6878, then reverted because we were not yet in
privatepreview (https://preview.portal.azure.com/). Commit 74392a8 to mark Clusters_Delete as a long running operation was the only change.I received my v- Microsoft credentials yesterday.
I'll see if I can access the OpenAPI Hub.(I do not.)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.