-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
[Hub Generated] Azure Site Recovery (ASR) - Azure Disk Encryption OnePass support changes. #6742
Conversation
In Testing, Please Ignore[Logs] (Generated from 92fc22a, Iteration 4).NET: test-repo-billy/azure-sdk-for-net [Logs] [Diff]
Python: test-repo-billy/azure-sdk-for-python [Logs]
Java: test-repo-billy/azure-sdk-for-java [Logs] [Diff]
Go: test-repo-billy/azure-sdk-for-go [Logs] [Diff]JavaScript: test-repo-billy/azure-sdk-for-js [Logs] [Diff]
Ruby: test-repo-billy/azure-sdk-for-ruby [Logs] [Diff]
|
Automation for azure-sdk-for-pythonNothing to generate for azure-sdk-for-python |
Automation for azure-sdk-for-goNothing to generate for azure-sdk-for-go |
Can one of the admins verify this patch? |
ModelValidationFailures are nothing to do with the change we have introduced as part of this PR / Commit. |
@@ -7892,6 +7892,14 @@ | |||
"description": "The fabric specific object Id of the virtual machine.", | |||
"type": "string" | |||
}, | |||
"initialPrimaryFabricLocation": { |
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.
add new properties in stable version better to be in a new api version.
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.
These new properties do not break any existing flows / contracts, we would like to handle in latest API version itself.
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.
These changes are considered a breaking change, a new API version is required.
Take a look at 'New property added to response' for why this is required:
https://microsoft.sharepoint.com/:w:/t/azureresourcemanagerteam/EWXsAQ1yx25KkyYCeeWGUgwBSBxEdUDEbHi6FZ__U8EOQw?rtime=WPZkgiIc10g
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.
- As per referred "New property added to response" in RestAPI_BreakingChangeGuide, it is considered breaking if "customer does a PUT using the model that was returned from the GET", which does not apply for our GET / PUT ReplicationProtectedItem as property names are different (ref: GET ReplicationProtectedItem, PUT ReplicationProtectedItem).
- These newly added properties are option (not required properties). As per PR Updating service.json to add few properties to VMNicDetails and VMNicInputDetails #6380, it is required to bump up the API version only on adding required properties.
- We are referring "Definition of a breaking change" from "Microsoft REST API Guidelines" - where it is said that "Services MUST explicitly define their definition of a breaking change, especially with regard to adding new fields to JSON responses and adding new API arguments with default fields." - In our case, we define the current changes as not breaking change.
- Please share some best practices to be followed within service to implement, handle, and maintain multiple versions for every optional property we introduce (our service version handling is here).
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.
IN section 12.3, https://github.com/microsoft/api-guidelines/blob/vNext/Guidelines.md#123-definition-of-a-breaking-change, it is explicitly stated -
For example, Azure defines the addition of a new JSON field in a response to be not backwards compatible.
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.
The guidance is to bundle all such changes so that every optional property addition does not require a new api-version.
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.
please mark this as readonly since its not settable
Who from ARM Team will be reviewing for label WaitForARMFeedback? |
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.
Need update examples?
}, | ||
"diskEncryptionInfo": { | ||
"$ref": "#/definitions/DiskEncryptionInfo", | ||
"description": "The recovery disk encryption information (for one / single pass flows)." |
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.
With the new properties update, please also update the examples for references.
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.
@yaohaizh - Azure Site Recovery (ASR) service supports multiple replication providers and following are published providers (under ReplicationProviderSpecificSettings): * A2AReplicationDetails * HyperVReplicaAzureReplicationDetails * HyperVReplicaBlueReplicationDetails * HyperVReplicaReplicationDetails * InMageAzureV2ReplicationDetails * InMageReplicationDetails
- All the resource operations examples are with one such provider "HyperVReplicaAzure".
- And the new properties belong to "A2A".
We cannot update the examples.
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.
For documentation purposes, you should include examples for each of these.
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.
- Are you suggesting us to include examples for all the above shared provider specific properties? If so, we would like to take as a sperate checkin.
PS: The new properties in this PR belong to one provider (Azure To Azure) and have no relation with the provider (HyperV to Azure) mentioned in the examples. - Will it be okay if we give examples for all the providers and all the properties - it will be a very very big thing - we have multiple properties (multiple provider specific properties with almost all operations). Please share some complex service examples for our reference & next steps.
- Few of the properties are mutual exclusive, do we need to give multiple examples within a provider for all the mutual exclusive properties?
- Is there anyone who can review in IDC (or IDC time friendly office hours)? So that we can expedite the review.
@mybayern1974, @yaohaizh, @yungezz - please let us know who needs to review this PR. |
The PR is pending ARM feedback. @yaohaizh, could you pls take care of this PR? Thanks. |
@yungezz asked for ARM review board. |
Reasking the same question after 6 days "Who from ARM Team will be reviewing for label WaitForARMFeedback?" PS: label WaitForARMFeedback got added 9 days back. @yungezz - please share any individual mail or DL to whom we can request for review wrt label WaitForARMFeedback. |
@sriramvu - @ryansbenson from ARM review board has reviewed this and added comments. |
Yes Gaurav, we have responded to @ryansbenson - waiting for guidance wrt the questions asked. |
Hi @ravbhatnagar could you pls help to continue ARM review since @ryansbenson is oof, while service team is urgent for the PR merging. |
there's semantic and model validation error at https://dev.azure.com/azure-sdk/public/_build/results?buildId=90117&view=logs&j=95dbed5b-845d-57ba-33d3-f6c5e69afcf4 , pls fix them. |
As per discussion from Punit Bhatt, it seems like the second put which does not contain this property will not over write the property state on the server, it will take the value from the DB. User can only reconfigure/reset properties during the UpdateVM call (2). Here, he can modify the values or reset them back to null |
"description": "The initial primary fabric location.", | ||
"type": "string" | ||
}, | ||
"initialRecoveryFabricLocation": { |
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.
this also looks like readonly
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.
It is a read only property. Made appropriate changes.
@@ -8043,6 +8051,10 @@ | |||
"recoveryAvailabilityZone": { | |||
"description": "The recovery availability zone.", | |||
"type": "string" | |||
}, | |||
"vmEncryptionType": { |
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.
does it have an allowed set of values? can we make this enum?
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.
Changed this and added a set of allowed values.
diskEncryptionInfo - for encrypted VMs, if this is missing in second put, Service will fail. for unencrypted VMs, if this is not present on second put, its ignored. So will not lead to breaking change. For 2 properties, they are readonly and Punit will update. encryptionType will be made as enum. |
…-Microsoft.RecoveryServices-2018-07-10-OnePassAdeChanges Resolving Azure/azure-rest-api-specs Azure#6742 PR comments
Signing off from ARM side. |
If you are a MSFT employee you can view your work branch via this link.
Contribution checklist: