-
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
Service Fabric - add managedcluster and nodetype specs 2020-01-01-preview #10085
Service Fabric - add managedcluster and nodetype specs 2020-01-01-preview #10085
Conversation
Swagger pipeline can not start as the pull request has merge conflicts. |
Pull request contains merge conflicts. |
Can one of the admins verify this patch? |
e03c7f6
to
6ed51c1
Compare
[Staging] Swagger Validation Report
️✔️ |
Azure Pipelines successfully started running 1 pipeline(s). |
Trenton Generation - 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
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 CLI Extension Generation - 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-python - Release
|
azure-sdk-for-go - Release
|
azure-sdk-for-python-track2 - 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
|
azure-sdk-for-net - Release
|
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines successfully started running 1 pipeline(s). |
}, | ||
"adminPassword": { | ||
"type": "string", | ||
"format": "password", |
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.
"format": "password [](start = 0, length = 29)
Please go through https://armwiki.azurewebsites.net/rp_onboarding/process/api_review_best_practices.html?q=best%20practice
In this case please annotate using 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.
done thanks
...vicefabric/resource-manager/Microsoft.ServiceFabric/preview/2020-01-01-preview/nodetype.json
Show resolved
Hide resolved
...vicefabric/resource-manager/Microsoft.ServiceFabric/preview/2020-01-01-preview/nodetype.json
Show resolved
Hide resolved
...vicefabric/resource-manager/Microsoft.ServiceFabric/preview/2020-01-01-preview/nodetype.json
Show resolved
Hide resolved
...manager/Microsoft.ServiceFabric/preview/2020-01-01-preview/examples/DeleteNodes_example.json
Show resolved
Hide resolved
Azure Pipelines successfully started running 1 pipeline(s). |
@chiragg4u thanks for reviewing. I have fixed some of the comments and added a response to others, could you take a look again? |
Thanks for taking the comments. In general the changes looks like you are modeling ServiceFabric nodes which will contain VMSS nodes. AFAIK VMSS team is also planning to come up the tracked resource for VMSS object, and I've few questions
|
...-manager/Microsoft.ServiceFabric/preview/2020-01-01-preview/examples/Operations_example.json
Show resolved
Hide resolved
The idea of this new model is for SFRP to manage the entire resource group for the essential components of the service fabric cluster that includes VMSS, VNet/Load Balancer and storage account. In the Previous model SFRP only had control over the ‘cluster’ resource that was mostly configurations about the cluster but all the other resources (VMSS / VNet / etc) where the actual cluster runs on was managed by the customer and had to be in sync. |
@leni-msft this has been signed off by ARM can we merge the pr, please? |
"format": "password", | ||
"description": "vm admin user password." | ||
}, | ||
"loadBalancingRules": { |
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.
@a-santamaria this is not a valid array declaration and is breaking our tooling currently. Also, what did the generated SDK look like for this property and the ones below?
Take a look at https://swagger.io/docs/specification/data-models/data-types/#array to see how arrays are supposed to work in Swagger.
@leni-msft How did this get past the linters?
"$ref": "#/definitions/LoadBalancingRule", | ||
"description": "Describes a load balancing rule." | ||
}, | ||
"clients": { |
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 has a similar problem as loadBalancingRules. Same for any other property of type array
with $ref
at the same level.
…view (Azure#10085) * adding managedcluster and nodetype specs * remove name form put requests * fix descriptions * fix typos * run prettier-fix * change api version to 2020-01-01-preview * rename vmSku parameter to vmSize * fix fqdn and clusterCertificateCommonName in examples * fix comments and update with latest * prettier fixes and fix typos * renaming example to see if validtion can find it * add operations example and secret tag to password Co-authored-by: Alfredo Santamaria Gomez <alsantam@microsoft.com>
MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.
Contribution checklist:
If any further question about AME onboarding or validation tools, please view the FAQ.
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.