Microsoft.Web - Add snapshots, deleted apps, app recovery

[nking-1]

This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.

PR information

• The title of the PR is clear and informative.
• There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
• Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
• If applicable, the PR references the bug/issue that it fixes.
• Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

• I have read the contribution guidelines.
• My spec meets the review criteria:
  • The spec conforms to the Swagger 2.0 specification.
  • The spec follows the guidelines described in the Swagger checklist document.
  • Validation tools were run on swagger spec(s) and have all been fixed in this PR.

[nking-1]

There will be some validation errors / warnings that are expected. Please refer to another currently open pull request for this spec at #1551 for a discussion of those errors.

[azuresdkciprbot]

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/web/resource-manager/readme.md
Before the PR: Warning(s): 1637 Error(s): 98
After the PR: Warning(s): 1624 Error(s): 97

AutoRest Linter Guidelines | AutoRest Linter Issues

Send feedback and make AutoRest Linter Azure Bot smarter day by day!

Thanks for your co-operation.

[jhendrixMSFT]

I'm reviewing this today.

[ravbhatnagar]

@Nking92 I see a bunch of removals which looks like breaking changes. This would need a new api-version. Plus, you cant stop supporting GA APIs without any deprecation notice to customers.

[nking-1]

Outside of our swagger specs this feature is unannounced and not GA. The old version of this API will no longer work. I can still create a new API version if we need to.

[salameer]

@Nking92, Please note that there are SDK that are released on top of these swaggers and removing this operation breaks customers that have already built dependency on them

The swagger has to represent the Status of the API. if you swaggers are representing preview feature then add a -preview flag to the version folder of your swagger

[jhendrixMSFT]

What are the next steps here, will a new API version be created and the existing functionality be preserved?

[nking-1]

The backend for the old API has been completely gutted from our production service since it had many crucial design flaws. I'm not worried about breaking customers who took a dependency on this particular feature, because the old API no longer works. Moving forward I'd prefer to completely revise our old API instead of creating a new API version, if that's acceptable.

[ravbhatnagar]

@Nking92 - So there are APIs being removed in DeletedWebApps.json file which, from what I understand, are not supported on the service side. These are non-GA APIs and so the customer impact should be minimal. I checked the traffic and there is no external traffic in the last 45 days. So we should be ok in removing the API in this file. Correct me if I am wrong on this.
Now, there are more changes in the other files which relates to adding new properties/models. What are these about and shouldnt those result in a new API-version?

[nking-1]

@ravbhatnagar Here are some of the technical details behind the change

The recovery API was changed to use a long-running operation pattern that is compliant with ARM. In the past it returned HTTP 200 with an operation ID packed into the response body. Now it returns HTTP 202 with the operation resource in the location header. This is a breaking change, but this particular API has only been intended for internal use until we announce the feature is ready for preview.

A few additional fields were added into the objects used to create website recovery requests. Those should not be breaking changes.

[jhendrixMSFT]

It looks like some of the new properties being added are required which would be a breaking change yes?

[nking-1]

I agree that those would technically be breaking changes, however in the interest of getting the feature ready for an upcoming deadline, it would be helpful to not create a new API version. I checked ARM logs and only found 2 instances of this API being called externally, with both calls failing. I feel that it's safe to assume we won't be breaking any customer code with this change.

Additionally, this version of the spec will be what I use to create the .NET SDK and Powershell commands for the feature. Since the feature hasn't been surfaced in any way except for the prior spec, I don't feel much risk in breaking customer code with this update. However if we decide that it's too risky to make this change as it is, I can do my best to update with a new API version.

[jhendrixMSFT]

@Nking92 I think this sounds reasonable given the ARM metrics. Thanks for bearing with us.

[AutorestCI]

Azure/azure-sdk-for-python#1402

[AutorestCI]

No modification for AutorestCI/azure-sdk-for-node

[AutorestCI]

Azure/azure-sdk-for-ruby#939