[Hub Generated] Review request for Microsoft.Insights to add version stable/2021-05-01 #16037
Conversation
|
Hi, @ToddKingMSFT Thanks for your PR. I am workflow bot for review process. Here are some small tips. Any feedback about review process or workflow bot, pls contact swagger and tools team. vsswagger@microsoft.com |
|
[Call for Action] To better understand Azure service dev/test scenario, and support Azure service developer better on Swagger and REST API related tests in early phase, please help to fill in with this survey https://aka.ms/SurveyForEarlyPhase. It will take 5 to 10 minutes. If you already complete survey, please neglect this comment. Thanks. |
Swagger Validation Report
|
| Rule | Message |
|---|---|
| OperationId should contain the verb: 'metrics' in:'Metrics_ListAtSubscriptionScopePost'. Consider updating the operationId Location: Microsoft.Insights/stable/2021-05-01/metrics_API.json#L119 |
|
| Booleans are not descriptive and make them hard to use. Consider using string enums with allowed set of values defined. Property: isDimensionRequired Location: Microsoft.Insights/stable/2021-05-01/metricDefinitions_API.json#L285 |
|
| Booleans are not descriptive and make them hard to use. Consider using string enums with allowed set of values defined. Property: autoAdjustTimegrain Location: Microsoft.Insights/stable/2021-05-01/metrics_API.json#L334 |
|
| Booleans are not descriptive and make them hard to use. Consider using string enums with allowed set of values defined. Property: validateDimensions Location: Microsoft.Insights/stable/2021-05-01/metrics_API.json#L338 |
|
| Booleans are not descriptive and make them hard to use. Consider using string enums with allowed set of values defined. Property: isDataAction Location: Microsoft.Insights/stable/2021-05-01/operations_API.json#L100 |
|
| Booleans are not descriptive and make them hard to use. Consider using string enums with allowed set of values defined. Property: fillGapWithZero Location: Microsoft.Insights/stable/2021-05-01/operations_API.json#L250 |
|
| Booleans are not descriptive and make them hard to use. Consider using string enums with allowed set of values defined. Property: toBeExportedForShoebox Location: Microsoft.Insights/stable/2021-05-01/operations_API.json#L272 |
|
| Booleans are not descriptive and make them hard to use. Consider using string enums with allowed set of values defined. Property: isHidden Location: Microsoft.Insights/stable/2021-05-01/operations_API.json#L276 |
The following errors/warnings exist before current PR submission:
| Rule | Message |
|---|---|
| Based on the response model schema, operation 'Operations_List' might be pageable. Consider adding the x-ms-pageable extension. Location: Microsoft.Insights/stable/2021-05-01/operations_API.json#L41 |
|
| Booleans are not descriptive and make them hard to use. Consider using string enums with allowed set of values defined. Property: isDimensionRequired Location: Microsoft.Insights/stable/2021-05-01/metricDefinitions_API.json#L201 |
|
| The x-ms-code-generation-setting extension is being deprecated. Please remove it and move settings to readme file for code generation. Location: Microsoft.Insights/stable/2021-05-01/metrics_API.json#L7 |
|
| The x-ms-code-generation-setting extension is being deprecated. Please remove it and move settings to readme file for code generation. Location: Microsoft.Insights/stable/2021-05-01/metricDefinitions_API.json#L7 |
|
| The x-ms-code-generation-setting extension is being deprecated. Please remove it and move settings to readme file for code generation. Location: Microsoft.Insights/stable/2021-05-01/operations_API.json#L7 |
️⚠️Avocado: 1 Warnings warning [Detail]
| Rule | Message |
|---|---|
| The default tag contains multiple API versions swaggers. readme: specification/monitor/resource-manager/readme.md tag: specification/monitor/resource-manager/readme.md#tag-package-2021-04 |
️️✔️ModelValidation succeeded [Detail] [Expand]
Validation passes for ModelValidation.
️️✔️SemanticValidation succeeded [Detail] [Expand]
Validation passes for SemanticValidation.
️⚠️Cross-Version Breaking Changes: 10 Warnings warning [Detail]
- Compared Swaggers (Based on Oad v0.9.0)
- current:stable/2021-05-01/metricDefinitions_API.json compared with base:stable/2018-01-01/metricDefinitions_API.json
- current:stable/2021-05-01/metricDefinitions_API.json compared with base:preview/2017-09-01-preview/metricDefinitions_API.json
- current:stable/2021-05-01/metrics_API.json compared with base:stable/2019-07-01/metrics_API.json
- current:stable/2021-05-01/metrics_API.json compared with base:preview/2017-09-01-preview/metrics_API.json
- current:stable/2021-05-01/operations_API.json compared with base:stable/2019-07-01/operations_API.json
- current:stable/2021-05-01/operations_API.json compared with base:preview/2017-09-01-preview/operations_API.json
| Rule | Message |
|---|---|
| The new version has a different type 'object' than the previous one ''. New: Microsoft.Insights/stable/2021-05-01/metricDefinitions_API.json#L213:9 Old: Microsoft.Insights/stable/2018-01-01/metricDefinitions_API.json#L187:9 |
|
| The new version has a different type 'object' than the previous one ''. New: Microsoft.Insights/stable/2021-05-01/metricDefinitions_API.json#L257:11 Old: Microsoft.Insights/stable/2018-01-01/metricDefinitions_API.json#L231:11 |
|
| The new version has a different type 'object' than the previous one ''. New: Microsoft.Insights/stable/2021-05-01/commonMonitoringTypes.json#L9:5 Old: Microsoft.Insights/stable/2018-01-01/metricDefinitions_API.json#L86:5 |
The following breaking changes are detected by comparison with latest preview version:
️️✔️CredScan succeeded [Detail] [Expand]
There is no credential detected.
️️✔️[Staging] SDK Track2 Validation succeeded [Detail] [Expand]
Validation passes for SDKTrack2Validation
- The following tags are being changed in this PR
️️✔️[Staging] PrettierCheck succeeded [Detail] [Expand]
Validation passes for PrettierCheck.
️️✔️[Staging] SpellCheck succeeded [Detail] [Expand]
Validation passes for SpellCheck.
️️✔️[Staging] Lint(RPaaS) succeeded [Detail] [Expand]
Validation passes for Lint(RPaaS).
|
Swagger pipeline restarted successfully, please wait for status update in this comment. |
|
Hi @ToddKingMSFT, Your PR has some issues. Please fix the CI sequentially by following the order of
|
|
Hi, @ToddKingMSFT your PR are labelled with WaitForARMFeedback. A notification email will be sent out shortly afterwards to notify ARM review board(armapireview@microsoft.com). |
openapi-workflow-bot
bot
added
the
WaitForARMFeedback
|
The python SDK breaking change gated check has been failing for months in this folder, there is another team looking into it. This change should not have affected it. I was trying to keep this to 2 commits but it looks like the OneAPI Hub automatically generates a few commits when setting up a new branch. You'll want to review the 5th commit as that is the one where the new API is introduced and new changes are made. The 6th commit was to fix an example violation after I incorporated feedback from the linter to change some property casing but forgot to update the casing in the example. |
| } | ||
| } | ||
| }, | ||
| "SubscriptionScopeMetricDefinitionCollection": { |
There was a problem hiding this comment.
Thanks for catching this. It is now used.
I assume metricDefinitions is not modeled as an Azure resource since it does not comply with Azure resource schema. (e.g. type is missing. name should be a string not an object). Is this correct? Refers to: specification/monitor/resource-manager/Microsoft.Insights/stable/2021-05-01/examples/GetMultiResourceMetricDefinitions.json:19 in 8f6fd9a. [](commit_id = 8f6fd9a, deletion_comment = False) |
| } | ||
| }, | ||
| "post": { | ||
| "tags": [ |
There was a problem hiding this comment.
What's the difference between GET and POST? Looks like there are returning the same result. Why do you need both? Usually POST is preferred to return metadata.
There was a problem hiding this comment.
Our existing metrics API just supports GET for fetching metric data so we wanted to keep that consistent, however because in this new API you can filter by resourceIds, you can quickly run into URL length issues so we also wanted to expose a POST version of the API to support filtering by a lot of resourceIds while avoiding URL length concerns.
If this isn't a common practice in ARM we could just go with the POST version, we just were trying to maintain some consistency with our existing API.
j5lim
added
the
ARMChangesRequested
| } | ||
| }, | ||
| "definitions": { | ||
| "LocalizableString": { |
There was a problem hiding this comment.
It seems that LocalizableString is defined in more than one file, please consider moving this object to a common place and referencing it from this new file. The place where it is defined again is in specification/monitor/resource-manager/Microsoft.Insights/stable/2021-05-01/metrics_API.json
There was a problem hiding this comment.
Created a commonMonitoringTypes.json file and moved it there.
| }, | ||
| "description": "Metric availability specifies the time grain (aggregation interval or frequency) and the retention period for that time grain." | ||
| }, | ||
| "MetricUnit": { |
There was a problem hiding this comment.
It seems that MetricUnit is defined in more than one file, please consider moving this object to a common place and referencing it from this new file. The place where it is defined again is in specification/monitor/resource-manager/Microsoft.Insights/stable/2021-05-01/metrics_API.json
There was a problem hiding this comment.
Created a commonMonitoringTypes.json file and moved it there.
| "value" | ||
| ] | ||
| }, | ||
| "ErrorContract": { |
There was a problem hiding this comment.
It seems also that ErrorContract is defined in more than one file, please consider moving this object to a common place and referencing it from this new file. The places where it is defined again are:
- specification/monitor/resource-manager/Microsoft.Insights/stable/2021-05-01/metrics_API.json
- specification/monitor/resource-manager/Microsoft.Insights/stable/2021-05-01/operations_API.json
There was a problem hiding this comment.
Created a commonMonitoringTypes.json file and moved it there.
| } | ||
| } | ||
| }, | ||
| "ErrorResponse": { |
There was a problem hiding this comment.
Created a commonMonitoringTypes.json file and moved it there.
|
Hello @paulomarquesc / @j5lim / @ruowan I've addressed the PR comments, can you take another look? |
| }, | ||
| "Post request for subscription level metric metadata": { | ||
| "$ref": "./examples/GetMultiResourceMetricMetadata.json" | ||
| } |
There was a problem hiding this comment.
The examples should be only for POST.
j5lim
added
ARMSignedOff
|
Hi @ruowan can I get your help merging this PR? The python breaking changes test is currently broken in this directory. These changes have not affected the that test. |
…stable/2021-05-01 (Azure#16037) * Adds base for updating Microsoft.Insights from version stable/2019-07-01 to version 2021-05-01 * Updates readme * Updates API version in new specs and examples * Copy 2018-01-01 metric definitions API and examples * Changes to add subscription scope metrics API * Fix post request example after fixing casing of parameter names * Addressed PR comments and moved common types to a common json file * Fix commonMonitoringTypes API version and formatting
…stable/2021-05-01 (Azure#16037) * Adds base for updating Microsoft.Insights from version stable/2019-07-01 to version 2021-05-01 * Updates readme * Updates API version in new specs and examples * Copy 2018-01-01 metric definitions API and examples * Changes to add subscription scope metrics API * Fix post request example after fixing casing of parameter names * Addressed PR comments and moved common types to a common json file * Fix commonMonitoringTypes API version and formatting
This is a PR generated at OpenAPI Hub. You can view your work branch via this link.
Changelog
Add a changelog entry for this PR by answering the following questions:
Contribution checklist:
If any further question about AME onboarding or validation tools, please view the FAQ.
ARM API Review Checklist
Otherwise your PR may be subject to ARM review requirements. Complete the following:
Check this box if any of the following apply to the PR so that label “WaitForARMFeedback” will be added automatically to begin ARM API Review. Failure to comply may result in delays to the manifest.
-[ ] To review changes efficiently, ensure you copy the existing version into the new directory structure for first commit and then push new changes, including version updates, in separate commits.
Ensure you've reviewed following guidelines including ARM resource provider contract and REST guidelines. Estimated time (4 hours). This is required before you can request review from ARM API Review board.
If you are blocked on ARM review and want to get the PR merged with urgency, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.
Breaking Change Review Checklist
If any of the following scenarios apply to the PR, request approval from the Breaking Change Review Board as defined in the Breaking Change Policy.
Action: to initiate an evaluation of the breaking change, create a new intake using the template for breaking changes. Addition details on the process and office hours are on the Breaking change Wiki.
Please follow the link to find more details on PR review process.