Skip to content

[Hub Generated] Review request for Microsoft.Insights to add version stable/2018-03-01 #12231

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.

Sign up for GitHub

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

Merged
akning-ms merged 11 commits into from
Jan 11, 2021
Merged

[Hub Generated] Review request for Microsoft.Insights to add version stable/2018-03-01 #12231

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
31b3f6e
Fix Azure Metric Alerts Swagger Correctness
yashmuel Dec 24, 2020
ee5fe0e
added suppresion rule and removed errorResponse
yashmuel Dec 29, 2020
a324955
removed isMigrated
yashmuel Dec 30, 2020
5d5183d
renamed suppression rule
yashmuel Jan 4, 2021
53e6412
trying to fix validation error
yashmuel Jan 4, 2021
05828a8
Another try to fix swagger
yashmuel Jan 4, 2021
581260d
trying to fix swagger validation
yashmuel Jan 4, 2021
6cb60ac
trying to pass swagger validation
yashmuel Jan 4, 2021
bee5cd2
fixed suppression
yashmuel Jan 5, 2021
57f1b8a
fixed space
yashmuel Jan 5, 2021
8b26648
Merge branch 'master' into dev-monitor-Microsoft.Insights-2018-03-01-…
yashmuel Jan 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions ...cation/monitor/resource-manager/Microsoft.Insights/stable/2018-03-01/metricAlert_API.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,13 @@
},
"204": {
"description": "No content: the request was successful, but the response is empty"
}
},
"default": {
"description": "BadRequest",
"schema": {
"$ref": "../../stable/2016-03-01/alertRules_API.json#/definitions/ErrorResponse"
Copy link
Member

@weidongxu-microsoft weidongxu-microsoft Jan 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems not a good practice to refer to another version (but as it is already done, there might not possible to fix within this version).

Also the response schema is not correct as in LintDiff. It should have an error property, which then contains code and message.
Though it is likely not possible to fix within same version, you might need to suppress.

Copy link
Member Author

@yashmuel yashmuel Jan 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weidongxu-microsoft right.
I've requested a suppression for the LintDiff and also mentioned it in the swagger.
What about the sdk check which fails, what should i do with this? suppression as well?

Copy link
Member

@weidongxu-microsoft weidongxu-microsoft Jan 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can ignore the CI that without "required" label. They does not block the merge.

As dotnet build, it appears from its log above, fails on some class as azure-sdk-for-net/sdk/monitor/Microsoft.Azure.Management.Monitor/tests/BasicTests/DataCollectionRulesTests.cs, so it is likely not caused by your PR.

Copy link
Member Author

@yashmuel yashmuel Jan 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weidongxu-microsoft cool, so is there anything that blocking me to merge this PR?

Another thing, I've another PR (#12234) with some error about Swagger ApiDocPreview failed, should i fixed this or it's like the sdk?

Copy link
Member

@weidongxu-microsoft weidongxu-microsoft Jan 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to re-run ApiDocPreview CI in the other PR. Anyway this does not block.

Both PR still have LintDiff, which blocks. I think perhaps the suppress is not done correctly. The correct one is perhaps

  - suppress: R4007

Copy link
Member

@weidongxu-microsoft weidongxu-microsoft Jan 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is his github ID. Though I do not think it will be exempted. And monitor had the json https://github.com/Azure/azure-rest-api-specs/blob/master/specification/monitor/resource-manager/Microsoft.Insights/stable/2015-04-01/operations_API.json, and it is used in current package-2019-06 tag.

Copy link
Member Author

@yashmuel yashmuel Jan 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weidongxu-microsoft
cool, i'll try him.

I've 2 additional PRs that i want to merge, but experiencing the same model validations we had in the current PR,
can you do the same process you did for this one to merge them?
#12371
#12372

Copy link
Member

@weidongxu-microsoft weidongxu-microsoft Jan 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For these you can also contact Arthur.

Copy link
Member Author

@yashmuel yashmuel Jan 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, will do.
but first you don't have to verify that the failing model validation are false alerts?

Copy link
Member Author

@yashmuel yashmuel Jan 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weidongxu-microsoft talked with Ray and suppressed the modelValidation, can you please help me merge this one?
#12371

}
}
},
"x-ms-examples": {
"Delete an alert rule": {
Expand Down Expand Up @@ -475,7 +481,6 @@
"MetricAlertProperties": {
"required": [
"enabled",
"description",
"severity",
"evaluationFrequency",
"windowSize",
Expand All @@ -488,6 +493,7 @@
},
"severity": {
"type": "integer",
"format": "int32",
"description": "Alert severity {0, 1, 2, 3, 4}"
},
"enabled": {
Expand Down Expand Up @@ -832,6 +838,7 @@
"description": "Namespace of the metric."
},
"timeAggregation": {
"type": "string",
"enum": [
"Average",
"Count",
Expand All @@ -840,7 +847,7 @@
"Total"
],
"x-ms-enum": {
"name": "AggregationType",
"name": "AggregationTypeEnum",
"modelAsString": true
},
"description": "the criteria time aggregation types."
Expand Down
3 changes: 3 additions & 0 deletions specification/monitor/resource-manager/readme.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -613,6 +613,9 @@ directive:
from: dataCollectionRuleAssociations_API.json
where: $.paths
reason: 'Operations API is defined in a separate swagger spec for Microsoft.Insights namespace (https://github.com/Azure/azure-rest-api-specs/blob/master/specification/monitor/resource-manager/Microsoft.Insights/stable/2015-04-01/operations_API.json)'
- suppress: DefaultErrorResponseSchemaMetricAlert_API
from: metricAlert_API.json
reason: 'Updating the error response to the new format would be a breaking change.'
```

### Tag: profile-hybrid-2019-03-01
Expand Down