Skip to content
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

Release sql microsoft.sql 2023 02 01 preview #25293

Merged
merged 20 commits into from
Aug 23, 2023

Conversation

jeremyfrosti
Copy link
Member

@jeremyfrosti jeremyfrosti commented Aug 14, 2023

ARM (Control Plane) API Specification Update Pull Request

PR review workflow diagram

Please understand this diagram before proceeding. It explains how to get your PR approved & merged.

diagram

[1] public repo review queue, private repo review queue
The PRs are processed by time opened, ascending. Your PR may show up on 2nd or later page.
If you addressed Step 1 from the diagram and your PR is not showing up in the queue, ensure the label ARMChangesRequested
is removed from your PR. This should cause the label WaitForARMFeedback to be added.
[2] https://aka.ms/azsdk/support/specreview-channel
[3] List of SDK breaking changes approvers in pinned Teams announcement
[4] public repo merge queue, private repo merge queue

If you need further help with anything, see Getting help section below.

Purpose of this PR

What's the purpose of this PR? Check all that apply. This is mandatory!

  • New API version. (Such PR should have been generated with OpenAPI Hub, per this wiki doc.)
  • Update existing version for a new feature. (This is applicable only when you are revising a private preview API version.)
  • Update existing version to fix swagger quality issues in S360.
  • Other, please clarify:
    • edit this with your clarification

Due diligence checklist

To merge this PR, you must go through the following checklist and confirm you understood
and followed the instructions by checking all the boxes:

Breaking changes review (Step 1)

  • If the automation determines you have breaking changes, i.e. Step 1 from the diagram applies to you,
    you must follow the breaking changes process.
    IMPORTANT This applies even if:
    • The tool fails while it shouldn't, e.g. due to runtime exception, or incorrect detection of breaking changes.
    • You believe there is no need for you to request breaking change approval, for any reason.
      Such claims must be reviewed, and the process is the same.

ARM API changes review (Step 2)

  • If this PR is in purview of ARM review then automation will add the ARMReview label.
  • If you want to force ARM review, add the label yourself.
  • Proceed according to the diagram at the top of this comment.

Getting help

jeremyfrosti and others added 18 commits May 19, 2023 18:03
* add mi refresh API

* remove stray character

* add examples

* fix lro error

* fix lro error only in example

* fixing prettier issue

* Test commit - adding error schema

* Test2

* Test - changing error type

* Revert "Test - changing error type"

This reverts commit 84fff94.

* Reverting 3 test commits

* Test - adding commong error type reference

* fix type format errors

---------

Co-authored-by: Stefan Krivokapic <[email protected]>
…r SQL DB Failover Group in version 2023-02-01-preview. (#24132)

* Added the swagger spec and example json files for failvoer group API update in V2023-020-1

* Corrected the FailoverGroupGet.json example

* Updated the auto-generated FailoverGroups.json

Fixed FailoverGroups.json swagger arm-id attribute for databases field

* Added the missing headers field manually

* Fix FailoverGroups.json

---------

Co-authored-by: Sharan Singh <[email protected]>
* Loc cap changes

* removing changes not related to my changes in dsmain
* add new dag api version

* Update swagger

* Update swagger

* patch resource

* make replicationMode writable
* Add FreeLimitExhaustion Capability in 2023-02-01-preview API

* Change property from name to exhaustionBehaviorType
…4305)

* Update swagger files for Server API

* Add v5 tag for Servers

* Restore none for minimal TLS version
* Add auto rotation param to databases api

* add armid
* Updated the examples for LTR Policies and LTR Backups

* Not sure why makeBackupsImmutable got deleted
* Carrying over minor changes for the 2023-02-01-preview release

* Reverting example files that didn't have DsMainDev side changes, and fixing prettier check

* Fixes for LTR examples with model validation and prettier check for ServerDelete

* Reverting LTR changes

* Adding arm-id to serversjson
@openapi-workflow-bot
Copy link

Hi, @jeremyfrosti! Thank you for your pull request. To help get your PR merged:

  • Ensure you reviewed the checklists in the PR description.
  • Know that PR assignee is the person auto-assigned and responsible for your current PR review and approval.
  • For convenient view of the API changes made by this PR, refer to the URLs provided in the table in the Generated ApiView comment added to this PR. You can use ApiView to show API versions diff.
  • @openapi-pipeline-app
    Copy link

    openapi-pipeline-app bot commented Aug 14, 2023

    Swagger Validation Report

    ️️✔️BreakingChange succeeded [Detail] [Expand]
    There are no breaking changes.
    ️❌Breaking Change(Cross-Version): 46 Errors, 51 Warnings failed [Detail]

    Only 0 items are rendered, please refer to log for more details.

    The following breaking changes are detected by comparison with the latest stable version:

    Only -1 items are listed, please refer to log for more details.

    Rule Message


    The following breaking changes are detected by comparison with the latest preview version:

    Only -1 items are listed, please refer to log for more details.

    Rule Message
    ️️✔️CredScan succeeded [Detail] [Expand]
    There is no credential detected.
    ️🔄LintDiff inProgress [Detail]
    ️️✔️Avocado succeeded [Detail] [Expand]
    Validation passes for Avocado.
    ️❌SwaggerAPIView: 0 Errors, 0 Warnings failed [Detail]
    ️️✔️TypeSpecAPIView succeeded [Detail] [Expand]
    ️❌ModelValidation: 302 Errors, 0 Warnings failed [Detail]

    Only -1 items are listed, please refer to log for more details.

    Rule Message
    ️️✔️SemanticValidation succeeded [Detail] [Expand]
    Validation passes for SemanticValidation.
    ️️✔️PoliCheck succeeded [Detail] [Expand]
    Validation passed for PoliCheck.
    ️️✔️PrettierCheck succeeded [Detail] [Expand]
    Validation passes for PrettierCheck.
    ️️✔️SpellCheck succeeded [Detail] [Expand]
    Validation passes for SpellCheck.
    ️️✔️Lint(RPaaS) succeeded [Detail] [Expand]
    Validation passes for Lint(RPaaS).
    ️️✔️PR Summary succeeded [Detail] [Expand]
    Validation passes for Summary.
    ️️✔️Automated merging requirements met succeeded [Detail] [Expand]
    Posted by Swagger Pipeline | How to fix these errors?

    @openapi-pipeline-app
    Copy link

    openapi-pipeline-app bot commented Aug 14, 2023

    Swagger Generation Artifacts

    ️️✔️ApiDocPreview succeeded [Detail] [Expand]

    Only 0 items are rendered, please refer to log for more details.

    ️❌SDK Breaking Change Tracking failed [Detail]

    Only 0 items are rendered, please refer to log for more details.

    ️⚠️ azure-sdk-for-python-track2 warning [Detail]

    Only 0 items are rendered, please refer to log for more details.

    ️❌ azure-sdk-for-net-track2 failed [Detail]

    Only 0 items are rendered, please refer to log for more details.

    ️⚠️ azure-sdk-for-java warning [Detail]

    Only 0 items are rendered, please refer to log for more details.

    ️️✔️ azure-sdk-for-go succeeded [Detail] [Expand]

    Only 0 items are rendered, please refer to log for more details.

    ️️✔️ azure-sdk-for-js succeeded [Detail] [Expand]

    Only 0 items are rendered, please refer to log for more details.

    ️️✔️ azure-resource-manager-schemas succeeded [Detail] [Expand]

    Only 0 items are rendered, please refer to log for more details.

    ️❌ azure-powershell failed [Detail]

    Only 0 items are rendered, please refer to log for more details.

    Posted by Swagger Pipeline | How to fix these errors?

    @openapi-pipeline-app
    Copy link

    openapi-pipeline-app bot commented Aug 14, 2023

    Generated ApiView

    Language Package Name ApiView Link
    Go sdk/resourcemanager/sql/armsql https://apiview.dev/Assemblies/Review/0dc0ec48f356435780ce485fb29ddb64
    JavaScript @azure/arm-sql https://apiview.dev/Assemblies/Review/0dcb8ffe723b4850ae864c9c1d658f4c

    @jeremyfrosti
    Copy link
    Member Author

    These commits have been approved during merging to the feature branch. Relevant commits: main...release-sql-Microsoft.Sql-2023-02-01-preview

    @azure-pipelines
    Copy link

    Azure Pipelines successfully started running 2 pipeline(s).

    @jeremyfrosti
    Copy link
    Member Author

    /azp run

    @azure-pipelines
    Copy link

    Azure Pipelines successfully started running 2 pipeline(s).

    @jeremyfrosti
    Copy link
    Member Author

    @TimLovellSmith The 'name' being writable has been used in previous versions, although the reliance on it has been removed in IPv6FirewallRules.json after version 2021-11-01-preview. We still can not simply update the ResourceWithWritableName to mark the property as readonly though, this same model is used in other files such as FirewallRules.json, and making it different in IPv6FirewallRules would cause us to have two different definitions with the same name, and we can't do that. It is also used for internal functions that rely on the name being writable.

    Theoretically, we can change ProxyResourceWithWritableName to ProxyResource here:

    But that may be a breaking change for downstream client generation. If its not, the change may be possible.

    Again though, does this need to be done for us to release our next version? This is not new to this version, and that api has had no changes in this version. It has existed for many versions before, so why are we blocking this one? This will cause a delay to our release, and that will cause us to miss deadlines on features that have been promised to our customers. If this must be blocked, we need to schedule a meeting to discuss this and understand why this is worth blocking our release and how we can expedite the change ASAP.

    @jeremyfrosti jeremyfrosti added WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required and removed ARMChangesRequested <valid label in PR review process>add this label when require changes after ARM review labels Aug 19, 2023
    @TimLovellSmith
    Copy link
    Member

    TimLovellSmith commented Aug 19, 2023

    @jeremyfrosti you're right it shouldn't really need to block feature development, I'm just trying to understand why we have these linter errors, and whether its the kind of API quality thing we can/should fix. In particular I'm puzzled about whether you are deliberately allowing the behavior today, maybe you could explain more about what 'reliance' or lack of it means, while I sign this off...

    @TimLovellSmith TimLovellSmith added the ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review label Aug 19, 2023
    @openapi-workflow-bot openapi-workflow-bot bot removed the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Aug 19, 2023
    @TimLovellSmith
    Copy link
    Member

    @kazrael2119
    Copy link
    Contributor

    this js sdk breaking is caused by this pr

    @Alancere Alancere added the Approved-SdkBreakingChange-Go Approve the breaking change tracking for azure-sdk-for-go label Aug 22, 2023
    * Carrying over minor changes for the 2023-02-01-preview release
    
    * Reverting example files that didn't have DsMainDev side changes, and fixing prettier check
    
    * Fixes for LTR examples with model validation and prettier check for ServerDelete
    
    * Reverting LTR changes
    
    * Adding arm-id to serversjson
    
    * Removing the Pattern property from ManagedInstances.json as it should not be present and will cause issues in downstream client generation
    @openapi-pipeline-app
    Copy link

    Automatic PR validation started. This comment will be populated with next steps to merge this PR once validation is completed. Please wait ⌛.

    @ms-henglu
    Copy link
    Member

    I will approve and unblock this for a couple contributing reasons.

    1. The previous PR was approved and released with 320 errors. This PR actually narrows those errors from 320 to 302!
    2. All errors in this PR are this INVALID_TYPE error. As far as I can tell, I believe this is an oav analysis issue. Specifically, I have filed a new issue against Azure/oav to cover a resolution on analysis side.

    Given the above situation, I'm going to approve failing ModelValidation check here.

    According to this comment, I'll help adding the Approved-ModelValidation label.

    @ms-henglu
    Copy link
    Member

    The one new lint diff error detected actually exists in previous version.

    @jeremyfrosti
    Copy link
    Member Author

    /pr RequestMerge

    @ms-henglu ms-henglu merged commit c942bdc into main Aug 23, 2023
    @ms-henglu ms-henglu deleted the release-sql-Microsoft.Sql-2023-02-01-preview branch August 23, 2023 05:06
    @openapi-pipeline-app
    Copy link

    Swagger pipeline restarted successfully, please wait for status update in this comment.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Approved-BreakingChange DO NOT USE! OBSOLETE label. See https://github.com/Azure/azure-sdk-tools/issues/6374 Approved-LintDiff Approved-ModelValidation Approved-SdkBreakingChange-Go Approve the breaking change tracking for azure-sdk-for-go Approved-SdkBreakingChange-JavaScript Approved-SdkBreakingChange-Python ARMReview ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review BreakingChangeReviewRequired <valid label in PR review process>add this label when breaking change review is required CI-BreakingChange-Go CI-BreakingChange-JavaScript CI-FixRequiredOnFailure CI-MissingBaseCommit new-api-version resource-manager
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.