[Hub Generated] Review request for Microsoft.Insights to add version stable/2023-10-01 #26655
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Next Steps to Merge✔️ All automated merging requirements have been met! Refer to step 4 in the PR workflow diagram (even if your PR is for data plane, not ARM). |
Swagger Generation Artifacts
|
Generated ApiView
|
This was referenced Nov 10, 2023
AzureRestAPISpecReview
added
BreakingChangeReviewRequired
AzureRestAPISpecReview
added
ARMChangesRequested
|
Please address or respond to feedback from the ARM API reviewer. |
ToddKingMSFT
removed
the
ARMChangesRequested
openapi-workflow-bot
bot
added
the
WaitForARMFeedback
@raosuhas If the issue is the diff view I've manually created the equivalent ARM diff views for the metrics and metricDefinitions API diffs here. This PR has already gone through multiple breaking change meetings and an hour plus long stewardship board review. I don't want to restart the whole process with 2 parallel PRs. I can setup some time to meet with you to help go over the changes if you still need help diffing them. You can also do this yourself you'll want to diff the 2023-10-01 arm changes found in this PR with the exsiting 2021-05-01 arm metrics APIs this PR is based on. metricDefinitions review: |
raych1
added
the
Approved-SdkBreakingChange-Go
|
@ToddKingMSFT duplicated schemas are referenced in the swaggers included in the default tag in readme.md. This causes the duplicated names are renamed by adding |
raych1
removed
the
Approved-SdkBreakingChange-Go
|
Hi @ToddKingMSFT , in the default tag: 1 duplicate schema 2 duplicate schemas 3 duplicate schema 4 duplicate schema 5 duplicate schema 6 duplicate schemas 7 duplicate schema 8 duplicate schema 9 duplicate schema And duplicate schema 10 duplicate schema please ask related team to consolidate the references. thanks |
I will try to resolve what I can but the majority of this is from other team's swaggers that I don't have control over. In the past when I have tried to make small tweaks to these swaggers that has suddenly opened the flood gates for a bunch of new lint and other violations that now need to be fixed if I touch that swagger and the whole PR snowballs. |
sjanamma
reviewed
Nov 30, 2023
| } | ||
| } | ||
| }, | ||
| "ErrorResponse": { |
There was a problem hiding this comment.
Can you use the common type Error Response?
https://github.com/Azure/azure-rest-api-specs/blob/main/specification/common-types/resource-management/v5/types.json
There was a problem hiding this comment.
Can you use the common type Error Response? https://github.com/Azure/azure-rest-api-specs/blob/main/specification/common-types/resource-management/v5/types.json
The problem is there are a bunch of APIs that are stuck with the old v1 error response that had everything in the root intead of nested in a "error" object like v2 and above. I can't use v5 as that would be a breaking change.
What I could do I think is use the old v1 ErrorResponse instead though.
sjanamma
added
the
ARMSignedOff
openapi-workflow-bot
bot
removed
the
WaitForARMFeedback
|
Hi @kazrael2119
To get more detailed here is all of your feeback with some inline comments from me: 1 duplicate schema LocalizableString from tenantActivityLogs_API.json is defined manually but the schema from metricDefinitions_API.json refenerces from common-type v2 9 duplicate schema PrivateEndpointConnection from monitoringAccounts_API.json references from common-type v4 but from /privateLinkScopes_API.json it references from common-type v2. And duplicate schemaPrivateEndpoint is a parameter of PrivateEndpointConnection in common-type definition. So both PrivateEndpoint and PrivateEndpointConnection are duplicate I see what you mean here and at initial glance it does look like a lot of thedefinitions in privateLinksScope_API could just be using the existing common-types definitions, but I’d rather have the private links team own this as it’s not an area I’m well familiar with. 10 duplicate schema TrackedResource is because different files reference from different common-type version: v1, v2 and v3. So it's duplicate This seems doable but touches a lot of other files that then introduce other errors so I would like to do this in a separate PR. |
|
What I'd like to get accross is Microsoft.Insights is made up of MANY MANY different teams in different timezones. I'm personally from the metrics team and am trying to update our metrics APIs but much of this feedback and errors are about problems with other team's swaggers. |
|
Hi @sjanamma can you also approve my suppressions? (Approved-Suppression tag?) |
|
approved for JS SDK BreakingChange as @ToddKingMSFT did his best to solve the duplicate schema issues he could solve in #26927. |
|
@jhendrixMSFT Can you help approve the suppressions I added and add the Approved-Suppression tag? |
|
/pr RequestMerge |
|
@jhendrixMSFT can you merge this please? |
visingla-ms
approved these changes
Dec 8, 2023
visingla-ms
deleted the
ToddKingMSFT-monitor-Microsoft.Insights-2023-10-01-DpAndARM
branch
arifibrahim4
pushed a commit
that referenced
this pull request
Feb 26, 2024
…stable/2023-10-01 (#26655) * Adds base for updating Microsoft.Insights from version preview/2023-05-01-preview to version 2023-10-01 * Updates readme * Updates API version in new specs and examples * Introduce the new 2023-10-01 metrics API versions * Update * Fix avacado * Fix up examples and body parameters * Fix example formatting * Remove paging * Fix prettier formatting and suppress lint rules that would cause a breaking change if addressed. * Make changes requested by stewardship board * update batch examples * Add x-ms-pageable back in * Update description * Just use the existing 2015-04-01 operations API all past APIs used. * use ErrorResponse from common-types v1 for APIs with legacy error response
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a PR generated at OpenAPI Hub. You can view your work branch via this link.
Data Plane API - Pull Request
API Info: The Basics
Most of the information about your service should be captured in the issue that serves as your API Spec engagement record.
Is this review for (select one):
Change Scope
This section will help us focus on the specific parts of your API that are new or have been modified.
Please share a link to the design document for the new APIs, a link to the previous API Spec document (if applicable), and the root paths that have been updated.
Viewing API changes
For convenient view of the API changes made by this PR, refer to the URLs provided in the table
in the
Generated ApiViewcomment added to this PR. You can use ApiView to show API versions diff.Suppressing failures
If one or multiple validation error/warning suppression(s) is detected in your PR, please follow the
Swagger-Suppression-Process
to get approval.
❔Got questions? Need additional info?? We are here to help!
Contact us!
The Azure API Review Board is dedicated to helping you create amazing APIs. You can read about our mission and learn more about our process on our wiki.
Click here for links to tools, specs, guidelines & other good stuff
Tooling
Guidelines & Specifications
Helpful Links