-
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
[Hub Generated] Review request for Microsoft.AzureStack to add version stable/2017-06-01 #8240
[Hub Generated] Review request for Microsoft.AzureStack to add version stable/2017-06-01 #8240
Conversation
Azure Pipelines successfully started running 1 pipeline(s). |
azure-sdk-for-go
|
azure-sdk-for-js
|
azure-sdk-for-java
|
azure-sdk-for-python
|
azure-sdk-for-net
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 Pipelines successfully started running 1 pipeline(s). |
1 similar comment
Azure Pipelines successfully started running 1 pipeline(s). |
Please review failures and ensure checks are passing. Without green check results, PR cannot be merged. Thx. |
...on/azurestack/resource-manager/Microsoft.AzureStack/stable/2017-06-01/CloudManifestFile.json
Outdated
Show resolved
Hide resolved
...on/azurestack/resource-manager/Microsoft.AzureStack/stable/2017-06-01/CloudManifestFile.json
Outdated
Show resolved
Hide resolved
Azure Pipelines successfully started running 1 pipeline(s). |
Please review check pipeline failures and make corrections. Thanks. |
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines failed to run 1 pipeline(s). |
@allenjzhang The checks are passing now. Could you please add the ARM folks as reviewers for sign off? |
/azp run automation - sdk |
Azure Pipelines successfully started running 1 pipeline(s). |
@@ -44,6 +44,53 @@ | |||
"nextLinkName": "nextLink" | |||
} | |||
} | |||
}, | |||
"/providers/Microsoft.AzureStack/cloudmanifestfile": { |
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.
cloudmanifestfile [](start = 37, length = 17)
As per the RPC, resource type names should be plural. If it's a singleton, the correct pattern would be to implement the following routes:
/providers/Microsoft.AzureStack/cloudmanifestfiles
/providers/Microsoft.AzureStack/cloudmanifestfiles/default
default
can be substituted with master
, current
, or latest
depending on the requirements.
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.
Fixed
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.
You need the list route also (/providers/Microsoft.AzureStack/cloudmanifestfiles
). Yes, I know it's weird to have it just for 1 result, but that's the pattern.
In reply to: 379690800 [](ancestors = 379690800)
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.
Do I have to implement the API for list? We don't have a use case for 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.
Yeah :(. GET verb in ARM RPC is reserved for resources, which for a read-only resource like this would require LIST and GET at minimum. The other alternative would be to make it a POST action.
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.
For some reason, I couldn't get proxy action work on tenant level. And this should not be a POST. I'll probably implement the list same as GET default.
...urestack/resource-manager/Microsoft.AzureStack/stable/2017-06-01/examples/Operation/Get.json
Outdated
Show resolved
Hide resolved
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.
Please take a look at the comments I added.
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
@AndyTao0402 - just one more minor comment.
@@ -44,6 +44,86 @@ | |||
"nextLinkName": "nextLink" | |||
} | |||
} | |||
}, | |||
"/providers/Microsoft.AzureStack/cloudmanifestfiles": { |
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 camel case the resource type name? cloudManifestFiles?
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.
is this not caught by linter? surprised
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.
Fixed
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines successfully started running 1 pipeline(s). |
ARM reviewers, could you please approve? |
signing off! |
…n stable/2017-06-01 (Azure#8240) * Update AzureStack API descriptions to include new GetCloudManifestFile API * Add CloudManifestFile example * Fix for spell check * Remove CloudName * Move new operation to AzureStack.json * Fix model error * Add one more optional query parameter * Address PR comments * Fix spell check * Add list operation * Fix style * Actually fix stykle * Use camel case name * Modify operation description * Fix Operation List example
If you are a MSFT employee you can view your work branch via this link.
Contribution checklist: