Skip to content

Comments

Policy Updates for new version#7699

Merged
erich-wang merged 17 commits intoAzure:masterfrom
pratimaupadhyay02:master
Dec 14, 2019
Merged

Policy Updates for new version#7699
erich-wang merged 17 commits intoAzure:masterfrom
pratimaupadhyay02:master

Conversation

@pratimaupadhyay02
Copy link
Contributor

@pratimaupadhyay02 pratimaupadhyay02 commented Nov 4, 2019

Latest improvements:

MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.

Contribution checklist:

  • I have reviewed the documentation for the workflow.
  • Validation tools were run on swagger spec(s) and have all been fixed in this PR.
  • The OpenAPI Hub was used for checking validation status and next steps.

ARM API Review Checklist

  • Service team MUST add the "WaitForARMFeedback" label if the management plane API changes fall into one of the below categories.
  • adding/removing APIs.
  • adding/removing properties.
  • adding/removing API-version.
  • adding a new service in Azure.

Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs.

  • If you are blocked on ARM review and want to get the PR merged urgently, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.
    Please follow the link to find more details on API review process.

@msftclas
Copy link

msftclas commented Nov 4, 2019

CLA assistant check
All CLA requirements met.

@AutorestCI
Copy link

AutorestCI commented Nov 4, 2019

Automation for azure-sdk-for-python

A PR has been created for you:
Azure/azure-sdk-for-python#9013

@AutorestCI
Copy link

AutorestCI commented Nov 4, 2019

Automation for azure-sdk-for-go

A PR has been created for you:
Azure/azure-sdk-for-go#6246

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@erich-wang erich-wang added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Nov 5, 2019
@ravbhatnagar
Copy link
Contributor

@pratimaupadhyay02 removing APIs from stable api-version is a breaking change. The Azure API deprecation process must be followed before APIs can be deprecated.

@pratimaupadhyay02
Copy link
Contributor Author

removing APIs from stable api-version is a breaking change. The Azure API deprecation process must be followed before APIs can be deprecated.

We are not removing this api version completely. We are just upgrading the api version with this SDK release. This is not deprecation.

@ravbhatnagar
Copy link
Contributor

not sure what the issue is. Is the GitHub diff messed up? It is showing bunch of APIs in red.

@adit-t
Copy link
Contributor

adit-t commented Nov 18, 2019

not sure what the issue is. Is the GitHub diff messed up? It is showing bunch of APIs in red.

Hey @ravbhatnagar . We are a little lost here. Are you saying we cannot move APIs across version from one SDK version to another? Just to reiterate, we are not removing these APIs. We are just bumping up their version. We had done this just a month back for more than 20 APIs. So, what's the issue you're talking about? Are we missing something here?

@erich-wang
Copy link
Member

@ravbhatnagar, could you please respond?

@ravbhatnagar
Copy link
Contributor

ravbhatnagar commented Nov 26, 2019

@pratimaupadhyay02 @TAdityaAnirudh - you can add apis to a newer api-version. But that does not mean you can remove apis from an existing api-version. If you want these apis to be supported in a newer api-version, create a new api-version and add them there. They must not be removed from existing GA api-version, which I am assuming these are.
I am assuming that you are not planning to deprecate the older api-version from where these APIs are being removed. Please correct me if my understanding is not accurate.
Does that make sense?

@pratimaupadhyay02
Copy link
Contributor Author

@pratimaupadhyay02 @TAdityaAnirudh - you can add apis to a newer api-version. But that does not mean you can remove apis from an existing api-version. If you want these apis to be supported in a newer api-version, create a new api-version and add them there. They must not be removed from existing GA api-version, which I am assuming these are. Does that make sense?

@ravbhatnagar We tried retaining the APIs in the existing GA-api version. Compilation of specs fails in that case. So it's not an option.
The changes in this PR are very similar to another PR we checked in around a month back.
https://github.com/Azure/azure-rest-api-specs/pull/7345/files
Has anything changed within this one month?
The older version is still active and is still supported. Any customer who uses an older version of Nuget can use that API version. We are not deprecating an API version with this. We just want our customers to use the latest version for these APIs. So, we are moving these around.
We can setup a call and discuss this if required.

@ravbhatnagar
Copy link
Contributor

please work with @sanjaiganesh as he is the current API review oncall.

@KrisBash
Copy link
Contributor

KrisBash commented Dec 5, 2019

@pratimaupadhyay02 please do not remove active APIs from prior versions. Users must be able to use all supported API versions in latest SDK builds

@azure-pipelines
Copy link

Pull request contains merge conflicts.

@azure-pipelines
Copy link

Pull request contains merge conflicts.

@azure-pipelines
Copy link

Pull request contains merge conflicts.

@azure-pipelines
Copy link

Comment was made before the most recent commit for PR 7699 in repo Azure/azure-rest-api-specs

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

This reverts commit 77c7bd5.
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@erich-wang
Copy link
Member

@KrisBash , please help to review again, thanks.

Copy link
Member

@anthony-c-martin anthony-c-martin left a comment

Choose a reason for hiding this comment

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

Please could you describe the changes you're making with this PR? When I diff bms.json against the previous version (2019-05-13), I see virtually no changes - is that intentional?

diff --git "a/2019-05-13\\bms.json" "b/2019-06-15\\bms.json"
index 24274a9..a77e080 100644
--- "a/2019-05-13\\bms.json"
+++ "b/2019-06-15\\bms.json"
@@ -1,7 +1,7 @@
 {
   "swagger": "2.0",
   "info": {
-    "version": "2019-05-13",
+    "version": "2019-06-15",
     "title": "RecoveryServicesBackupClient",
     "x-ms-code-generation-settings": {
       "internalConstructors": false
@@ -3141,6 +3141,17 @@
       },
       "x-ms-discriminator-value": "GenericProtectionPolicy"
     },
+    "InstantRPAdditionalDetails": {
+      "type": "object",
+      "properties": {
+        "azureBackupRGNamePrefix": {
+          "type": "string"
+        },
+        "azureBackupRGNameSuffix": {
+          "type": "string"
+        }
+      }
+    },
     "GenericProtectedItem": {
       "description": "Base class for backup items.",
       "type": "object",
@@ -4080,6 +4091,9 @@
     },
     "ProtectedItem": {
       "description": "Base class for backup items.",
+      "required": [
+        "protectedItemType"
+      ],
       "type": "object",
       "properties": {
         "protectedItemType": {

``` yaml $(tag) == 'package-2019-05'
input-file:
- Microsoft.RecoveryServices/stable/2019-05-13/bms.json
- Microsoft.RecoveryServices/stable/2019-06-15/bms.json
Copy link
Member

Choose a reason for hiding this comment

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

Please keep the old entry (2019-05-13) in-place

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Retained the old entry and created a new package (2019-06) which contains all the APIs.

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pratimaupadhyay02
Copy link
Contributor Author

Please could you describe the changes you're making with this PR? When I diff bms.json against the previous version (2019-05-13), I see virtually no changes - is that intentional?
diff --git "a/2019-05-13\bms.json" "b/2019-06-15\bms.json"
index 24274a9..a77e080 100644
--- "a/2019-05-13\bms.json"
+++ "b/2019-06-15\bms.json"
@@ -1,7 +1,7 @@
{
"swagger": "2.0",
"info": {

  • "version": "2019-05-13",
  • "version": "2019-06-15",
    "title": "RecoveryServicesBackupClient",
    "x-ms-code-generation-settings": {
    "internalConstructors": false
    @@ -3141,6 +3141,17 @@
    },
    "x-ms-discriminator-value": "GenericProtectionPolicy"
    },
  • "InstantRPAdditionalDetails": {
  •  "type": "object",
    
  •  "properties": {
    
  •    "azureBackupRGNamePrefix": {
    
  •      "type": "string"
    
  •    },
    
  •    "azureBackupRGNameSuffix": {
    
  •      "type": "string"
    
  •    }
    
  •  }
    
  • },
    "GenericProtectedItem": {
    "description": "Base class for backup items.",
    "type": "object",
    @@ -4080,6 +4091,9 @@
    },
    "ProtectedItem": {
    "description": "Base class for backup items.",
  •  "required": [
    
  •    "protectedItemType"
    
  •  ],
     "type": "object",
     "properties": {
       "protectedItemType": {
    

Yeah it is intentional. We are just adding the property RGNamePrefix and RGNameSuffix in AzureIaasVmProtectionPolicy and this contains the changes related to it only. Other than that we are bumping up the version of all the APIs to 2019-06-15.

@anthony-c-martin
Copy link
Member

Yeah it is intentional. We are just adding the property RGNamePrefix and RGNameSuffix in AzureIaasVmProtectionPolicy and this contains the changes related to it only. Other than that we are bumping up the version of all the APIs to 2019-06-15.

Thanks for the explanation! I see that InstantRPAdditionalDetails is being added as a definition but isn't being referenced anywhere (so I don't think it's going to have any effect on the shape of the API). Should it be referenced as a property on the AzureIaasVmProtectionPolicy definition?

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@anthony-c-martin anthony-c-martin added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Dec 13, 2019
Copy link
Member

@anthony-c-martin anthony-c-martin left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@erich-wang erich-wang merged commit d4f44d9 into Azure:master Dec 14, 2019
@ChenTanyi ChenTanyi mentioned this pull request Jan 9, 2020
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants