-
Notifications
You must be signed in to change notification settings - Fork 5.4k
Mysql version uniform #35110
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
base: main
Are you sure you want to change the base?
Mysql version uniform #35110
Conversation
…de all versions to a consistent version
Next Steps to MergeNext steps that must be taken to merge this PR:
|
PR validation pipeline restarted successfully. If there is ApiView generated, it will be updated in this comment. |
API Change CheckAPIView identified API level changes in this PR and created the following API reviews
|
@judyzhu10 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
@judyzhu10 we can ignore [TEST-IGNORE] - * failures for now. we need to move specification/mysql/resource-manager/Microsoft.DBforMySQL/readme.language.md into specification/mysql/resource-manager/Microsoft.DBforMySQL/FlexibleServer/readme.language.md, in this specification/mysql/resource-manager/Microsoft.DBforMySQL/FlexibleServer/readme.md, you only need to keep the input files reference to the uniformed swaggers. |
specification/mysql/resource-manager/Microsoft.DBforMySQL/legacy/readme.md
Outdated
Show resolved
Hide resolved
specification/mysql/resource-manager/Microsoft.DBforMySQL/FlexibleServers/readme.md
Outdated
Show resolved
Hide resolved
0bbd026
to
1e112b7
Compare
@mikeharder could you help take a look at the Avocado, ModelValidation, SchemaValidation failures? For model validation invalid format failure, For SemanticValidation failure, there's a internal tooling error. |
Avocado is failing for multiple reasons. There are so many errors, you need to view the full log: Most of the errors are
These other errors should be investigated more closely, and suggest potential real problems in the new readmes:
|
This can be suppressed in suppressions.yaml. |
@qiaozha, @JeffreyRichter: Should the |
Regex for arm-id in ModelValidation: |
SemanticValidation looks like a bug in the spec: This location defines I think the fix is to change the inline definition to a ref. |
Let me explain the background of this. First of all, MySQL has two services: legacy single server and flexible server, the legacy single server is going to retire, it's the flexible server that has the version uniform issue. For the legacy single server service, we will not spend any effort on the spec regarding either folder structure or the version uniform issue. For flexible server service, it has both version uniform issue and folder structure issue. the problem is that they have organized their service as a multiple service before https://github.com/Azure/azure-rest-api-specs/tree/main/specification/mysql/resource-manager/Microsoft.DBforMySQL but those are just operation groups within flexible server service not real multiple independent services. After the discussion with @akning-ms, we decide to go with the not split option, and remove those operation groups under flexible server services to be align with the folder structure v2. In this pr, we want to focus on the version uniform issue first so that it could unblock the service migrate to typespec. And folder structure for existing swagger is something we can fix later, that's why @judyzhu10 has moved all the existing swagger files into the legacy folder to avoid confusions. I am open to see if @mikeharder or @JeffreyRichter has better suggestions here? |
UPDATE: The current structure in main is pretty close to v2, with rp namespace and service names. only diff is one readme at rm, versus one readme per service:
structure in this PR, which is not valid per v2, because "legacy" would be the service name, then "AAD" is a sub-service which is not allowed per v2.
I understand the desire to minimize work on the legacy specs, but it should be done in a way that aligns as closely as possible with folder structure v2. |
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.
align with folder structure v2 as close as possible (at least as close as the current spec in main)
8e67d84
to
9df8b84
Compare
specification/mysql/resource-manager/Microsoft.DBforMySQL/common-types/v1/common-types.json
Outdated
Show resolved
Hide resolved
For the new Avocado failure, in FlexibleServers/preview/2024-12-01-preview/common-types.json you need to change the version into 2024-12-01-preview. For the UNREFERENCED_JSON_FILE and MISSING_APIS_IN_DEFAULT_TAG failures, you need to make sure all the files are included in resource-manager/Microsoft.DBforMySQL/FlexibleServers/readme.md |
@judyzhu10 could you unify the reference of OperationIdParameter here and Line 383 in ec239e3
|
@@ -1,7 +1,7 @@ | |||
{ | |||
"swagger": "2.0", | |||
"info": { | |||
"version": "2024-10-01-preview", | |||
"version": "2023-12-01-preview", |
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.
why we have this change?
For the TypeSpec requirement failure,
For the Swagger LintDiff error, could you add the suppression in the specification/mysql/resource-manager/Microsoft.DBforMySQL/FlexibleServers/readme.md for those errors? |
Though the SDK automation didn't report errors, but I think we need to clean up the unnessary configuration as the legacy service is no longger needed, there's no need to use tag to differentiate the flexible server service SDK in FlexibleServers readme.language.md.
Please remove readme.nodejs.md as it's no longer valid. @weidongxu-microsoft @XiaofeiCao for Java. |
@qiaozha Got it and updated for python. |
reason: "202 is a pattern that is already used in our existing resources and being carried forward to new implementations to maintain consistency for our customers. This has already been approved by the API review board." | ||
- code: PutInOperationName | ||
from: AdvancedThreatProtectionSettings.json | ||
reason: "This API is used to update thread detecion configuration, which is required by ARM policy, especially for `deployIfNotExist` scenario" | ||
- code: AllProxyResourcesShouldHaveDelete | ||
from: AdvancedThreatProtectionSettings.json | ||
reason: "PUT API is used to update thread detecion configuration, which is required by ARM policy, especially for `deployIfNotExist` scenario, we do not support DELETE operation" | ||
- code: PatchBodyParametersSchema | ||
from: swagger-document.json |
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.
from: swagger-document.json | |
from: swagger-document |
swagger-document is not a file, you don't need to add the .json file extension
Updated for Go. |
Choose a PR Template
Switch to "Preview" on this description then select one of the choices below.
Click here to open a PR for a Data Plane API.
Click here to open a PR for a Control Plane (ARM) API.
Click here to open a PR for only SDK configuration.