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.

Sign up for GitHub

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

Jump to bottom

WebApps - Add BackupName to backup API models #3560

Merged
merged 2 commits into from
Aug 20, 2018
Merged

WebApps - Add BackupName to backup API models #3560

merged 2 commits into from
Aug 20, 2018

Conversation

nking-1
Copy link

@nking-1 nking-1 commented Aug 1, 2018 •
edited by azuresdkci
Loading

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

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@azuresdkci azuresdkci requested a review from anuchandy August 1, 2018 00:07
@AutorestCI AutorestCI mentioned this pull request Aug 1, 2018
Merged
@AutorestCI
Copy link

AutorestCI commented Aug 1, 2018 •
edited
Loading

Automation for azure-sdk-for-python

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-python#2065

@AutorestCI
Copy link

AutorestCI commented Aug 1, 2018 •
edited
Loading

Automation for azure-sdk-for-ruby

Nothing to generate for azure-sdk-for-ruby

@AutorestCI AutorestCI mentioned this pull request Aug 1, 2018
Merged
@AutorestCI
Copy link

AutorestCI commented Aug 1, 2018 •
edited
Loading

Automation for azure-sdk-for-go

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-go#2485

@AutorestCI
Copy link

AutorestCI commented Aug 1, 2018 •
edited
Loading

Automation for azure-sdk-for-java

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-java#2218

This was referenced Aug 1, 2018
Merged
Closed
@AutorestCI
Copy link

AutorestCI commented Aug 1, 2018 •
edited
Loading

Automation for azure-sdk-for-node

A PR has been created for you based on this PR content.

Once this PR will be merged, content will be added to your service PR:
Azure/azure-sdk-for-node#3267

@anuchandy
Copy link
Member

Context: The type "BackupItem" has a "name" property in addition to the one it inherit from the it's base type "ProxyOnlyResource". This PR re-names the derived type's "name" property to "backupName".

From SDK side, when a type that derive from ARM base type has a property name conflicting with property from the base type, code-generator renames derived type's property. In this case a prefix "BackItem" will be used, so the property name become "BackupItemName". e.g. Java, C-Sharp. The changes in this PR is a breaking change, hence generated SDK's version needs to be bumpedup accordingly.

This is breaking change from the REST API side as well (Renaming property name in a stable api-version). hence requesting ARM review here.

@anuchandy anuchandy requested a review from ravbhatnagar August 8, 2018 17:05
@anuchandy anuchandy added WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required potential-sdk-breaking-change labels Aug 8, 2018
@nking-1
Copy link
Author

nking-1 commented Aug 8, 2018

We are ok with making a breaking SDK change. We have to make a major version update anyway. This PR was actually intended to make a workaround for other breaking changes, since the "name" property has already been removed from BackupRequest in an earlier PR. I made the same change to BackupItem here so that it will have the same workaround as BackupRequest.

@anuchandy
Copy link
Member

@Nking92 thanks for providing more info. Could you share the previously merged PR which fixes the same name issue in BackupRequest model?

@nking-1
Copy link
Author

nking-1 commented Aug 10, 2018

Hi @anuchandy, I checked and the name property was removed in the new 2018-02-01 API version. Here's that PR: #2802. Since it was removed in a new version, it wasn't actually a breaking change.

For my change here where I removed name from BackupItem, I actually only set that property be ignored from our generated swagger. It still exists and is used by our backend, so any clients of our REST API will continue to work as expected. I think the main concern would be breaking the SDKs, but they are already far overdue for a major version update anyway.

@anuchandy
Copy link
Member

thanks @Nking92. I see that BackupRequest::name property in previous api-version got removed when the new api-verison 2018-02-01 was introduced.

In this PR we have two changes:
1. Introducing a new property in BackupRequest model with name backupName.
2. Renaming BackupItem::name to backItem::backupName

@ravbhatnagar can you please take a look from ARM side?

@nking-1
Copy link
Author

nking-1 commented Aug 14, 2018

@anuchandy I reviewed code that uses our SDKs and determined that I won't need to change the name property of BackupItem to be BackupName for now. BackupItemName will be fine. The only important thing is that BackupRequest gets a BackupName property, because there is no alternative to set that property at the moment.

@ravbhatnagar
Copy link
Contributor

@Nking92 - this change should go in a new api-version. It can cause unintentional changes when used with strongly typed SDKs which will ignore the properties it doesnt understand.

@nking-1
Copy link
Author

nking-1 commented Aug 15, 2018

@ravbhatnagar Thanks for the review. Would only adding a new property for BackupRequest be ok? I have reverted the change that renames BackupItem::Name to BackupItem::BackupName.

@ravbhatnagar
Copy link
Contributor

@Nking92 - I was referring to that change only in my earlier comment. Take the case where this property was exposed in the current api-version and got set using the portal. And earlier version of the client SDK which is tied to the current api-version GETs the resource, makes some other changes to the properties, and PUTs the resource back. Now since the SDK did not understand this property, it will ignore it on the GET. And so this property wont be present in the request body on the PUT. Now your service may wipe the earlier set value of this property since it was not present in the request.
This is an unintentional update since the client ignore certain properties. So same SDK version causes behavior change in the same APi.
Anyway, its your call after this. Not sure how the service handles PUT when some properties are not present in the payload, does it override or ignore.
Signing off from ARM side.

@ravbhatnagar ravbhatnagar 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 Aug 16, 2018
@anuchandy
Copy link
Member

thank you @ravbhatnagar. @Nking92 let me know if you still want to make this change in the existing api version. As Gaurav pointed this change can cause existing SDK to behave differently than it used to be (of course depending on whether the service overwrite or ignore the value during PUT).

@nking-1
Copy link
Author

nking-1 commented Aug 17, 2018

@anuchandy I did some testing and I think we'll be ok. If the new property isn't present in the PUT request, the API will use the old property as expected. The new property does take precedence over the old property when it is present, though.

Signing off with OK to merge

@panchagnula
Copy link

@anuchandy can you please merge this PR please - we want to get our SDK changes rolling as well & we need this PR to be merged. Thank you.

@anuchandy anuchandy merged commit d800d9c into Azure:master Aug 20, 2018
sergey-shandar pushed a commit that referenced this pull request Aug 21, 2018
@MikhailTryakhov @sergey-shandar
Rebased master to monthly network branch (#3687)
aafc3d8 
* Swagger Coverage/Completeness- for operation CheckNameAvailability (#3663)

* Update service.json

* CheckNameAvailability

* FIxed Parameter name

fixed couple of parameters name

* Update dns management Node.js version number to 3.0.0 (#3675)

* Update batchai Node.js package version to 2.0.2 (#3664)

* Update nofiticationhubs management Node.js package version to 2.0.0 (#3665)

* WebApps - Add BackupName to backup API models (#3560)

* WebApps - Add BackupName to replace non-ARM-compliant Name property on Backup API models

* Revert breaking change to BackupItem. BackupItemName property will be fine in place of BackupName for now.

* Update authorization management Node.js package version to 5.0.1 (#3676)

* Update storagesync management Node.js package version to 1.0.0 (#3677)

* add checknameavailability (#3608)

* add checknameavailability

* move checkname parameter to definition section

* fix brackets

* fix parameter name in checknameavailability exanple

* update code owners for PowerBIDedicated

* New Batch data plane API version 2018-08-01.7.0 (#3657)

* Add new batch version 2018-08-01.7.0

* Update the new API

* Fix up output from OAD, it does not output valid JSON

* update OAV (#3678)

* Update storage management Node.js package version to 5.2.0 (#3679)

* Update compute.json and disk.json for some issues and add zone and rolling upgrade. (#3642)

* Breaking change tool correctly handles multiple files with same name

* Fix to breaking changes table generation

* Trusted Root certificate (#3668)
dsgouda pushed a commit that referenced this pull request Sep 11, 2018
@MikhailTryakhov @dsgouda
Local september branch merge to master (#3862)
42006c5 
* Trusted Root certificate (#3668)

* Rebased master to monthly network branch (#3687)

* Swagger Coverage/Completeness- for operation CheckNameAvailability (#3663)

* Update service.json

* CheckNameAvailability

* FIxed Parameter name

fixed couple of parameters name

* Update dns management Node.js version number to 3.0.0 (#3675)

* Update batchai Node.js package version to 2.0.2 (#3664)

* Update nofiticationhubs management Node.js package version to 2.0.0 (#3665)

* WebApps - Add BackupName to backup API models (#3560)

* WebApps - Add BackupName to replace non-ARM-compliant Name property on Backup API models

* Revert breaking change to BackupItem. BackupItemName property will be fine in place of BackupName for now.

* Update authorization management Node.js package version to 5.0.1 (#3676)

* Update storagesync management Node.js package version to 1.0.0 (#3677)

* add checknameavailability (#3608)

* add checknameavailability

* move checkname parameter to definition section

* fix brackets

* fix parameter name in checknameavailability exanple

* update code owners for PowerBIDedicated

* New Batch data plane API version 2018-08-01.7.0 (#3657)

* Add new batch version 2018-08-01.7.0

* Update the new API

* Fix up output from OAD, it does not output valid JSON

* update OAV (#3678)

* Update storage management Node.js package version to 5.2.0 (#3679)

* Update compute.json and disk.json for some issues and add zone and rolling upgrade. (#3642)

* Breaking change tool correctly handles multiple files with same name

* Fix to breaking changes table generation

* Trusted Root certificate (#3668)

* Updated Microsoft.Network version to 2018-08-01 (#3672)

* Adds base for updating Microsoft.Network from version stable/2018-07-01 to version 2018-08-01

* Updates readme

* Updates API version in new specs and examples

* Added AddressPrefixes on Subnet properties (#3714)

* Added address prefixes to subnet

* Added example of multiple address prefixes.

* Python Network conf 2018-08-01

* Changes to include required properties for outbound rule (#3728)

* Mark outbound rule properties as required

* Update outbound rule with required properties

* Removing old change

* Fix Python 2018-08 conf

* Use only PublicIpAddress in Azure Firewall, not both Public and InternalPublic (#3743)

* Do not confuse users with both InternalPublicIp and PublicIp for the same thing

* Update AzureFirewall PUT example

* This closes #3474

* Use the correct name for publicIPAddress in AzureFirewall PUT example

* Azure Firewall FQDN Tag top level resource (#3744)

* Azure Firewall FQDN Tag top level resource

* Name FqdnTags property in AzureFirewallApplicationRule properly

* Empty commit to trigger new validation

* Vtapfinal (#3684)

* Swagger Coverage/Completeness- for operation CheckNameAvailability (#3663)

* Update service.json

* CheckNameAvailability

* FIxed Parameter name

fixed couple of parameters name

* Adds base for updating Microsoft.Network from version stable/2018-07-01 to version 2018-08-01

* Updates readme

* Updates API version in new specs and examples

* vtap update

* VTAP changes

* VTAP changes

* Adding examples for VTAP resource

* Update changes for Examples

* Further fixes to examples

* Add exception for vtap's RequiredPropertiesMissingInResourceModel in readme

* Azure Firewall NAT Rule Collection (#3745)

* NAT Rule Collections for Azure Firewall

* Update examples to include the new NAT Rule Collection

* Shorten the NAT RC Properties name and mark etag read-only

* Add NAT Rule Collection to Azure Firewall properties

* Fix all <<a Azure>> in helper messages

* Do not allow ICMP as Network protocol (#3742)

* Adding linkedResourceType and hostedWorkloads to nic (#3780)

* Adding support for interface endpoints (#3783)

* Creating Interface Endpoint definition

* Adding references to interface endpoints in existing resources

* Added Trusted root certificate changes to new API version (#3731)

* Add ExpressRoute gateway resource (#3776)

* Initial version

* Refactor connections.

* Update API version

* Add enum values.

* Addressed all comments.

* Adding support for delegations on a subnet (#3805)

* Adding subnet delegation property on VNET

* Updating subnet/vnet create & get examples to reflect delegation

* Adding support for GetAvailableDelegations operation

* Fixing properties in Swagger & updating examples to reflect fixes

* Add availableDelegation as a subResource

* Remove type from examples

* Further updates to delegation PR

* Additional changes to structure

* Correct issues for SDK generation (#3818)

* Remove x-ms-client-flatten attribute as that results in wrong
descriptiosn during generation of the SDKs.

* Missing descriptions.

* Rename TargetUrls to TargetFqdns (#3739)

* Add Network Profile swagger and examples (#3806)

* add network profile swagger and examples

* add x-ms-client-flatten property as per feedback

* appease travis by fixing model + examples

* vtapswaggerupdate (#3837)

* Add Service Association Link and IP Configuration Profiles on Subnet (#3840)

* add service association links for virtual network

* add ipconfig profiles on subnet

* fixed example issue

* Review for tabfix and properties fromat rename

* Vtap swagger update and made the property as readonly (#3845)

* vtapswaggerupdate

* Adding readonly property

* Updated AppGW Autoscale Configuration (#3838)

* VPN gateway new API for Reset VPN client PSK (#3800)

* Brooklyn Ignite Swagger changes (#3715)

* Brooklyn Ignite Swagger changes

* Add missing example file changes

* Add virtual hub route table

* Fix travis build error

* VPN profile bugfix

* Fix casing for NRP automapping

* Fix parsing errors

* remove BgpSettings from P2SVpnGateway object

* Added AddressPrefixes on Subnet properties (#3714)

* Added address prefixes to subnet

* Added example of multiple address prefixes.

* Python Network conf 2018-08-01

* Changes to include required properties for outbound rule (#3728)

* Mark outbound rule properties as required

* Update outbound rule with required properties

* Removing old change

* Fix Python 2018-08 conf

* Fix swagger contract errors with NRP

* Fix network swagger

* Fix virtualHub Properties for SDK and PS JSON serializer compliance

* Fix bugs

* Changing Cortex swagger to match NRP

* Bug fixes

* bugFix3

* Bug fix

* Use only PublicIpAddress in Azure Firewall, not both Public and InternalPublic (#3743)

* Do not confuse users with both InternalPublicIp and PublicIp for the same thing

* Update AzureFirewall PUT example

* This closes #3474

* Use the correct name for publicIPAddress in AzureFirewall PUT example

* Azure Firewall FQDN Tag top level resource (#3744)

* Azure Firewall FQDN Tag top level resource

* Name FqdnTags property in AzureFirewallApplicationRule properly

* Empty commit to trigger new validation

* Vtapfinal (#3684)

* Swagger Coverage/Completeness- for operation CheckNameAvailability (#3663)

* Update service.json

* CheckNameAvailability

* FIxed Parameter name

fixed couple of parameters name

* Adds base for updating Microsoft.Network from version stable/2018-07-01 to version 2018-08-01

* Updates readme

* Updates API version in new specs and examples

* vtap update

* VTAP changes

* VTAP changes

* Adding examples for VTAP resource

* Update changes for Examples

* Further fixes to examples

* Add exception for vtap's RequiredPropertiesMissingInResourceModel in readme

* Azure Firewall NAT Rule Collection (#3745)

* NAT Rule Collections for Azure Firewall

* Update examples to include the new NAT Rule Collection

* Shorten the NAT RC Properties name and mark etag read-only

* Add NAT Rule Collection to Azure Firewall properties

* Fix all <<a Azure>> in helper messages

* Do not allow ICMP as Network protocol (#3742)

* Adding linkedResourceType and hostedWorkloads to nic (#3780)

* Adding support for interface endpoints (#3783)

* Creating Interface Endpoint definition

* Adding references to interface endpoints in existing resources

* Added Trusted root certificate changes to new API version (#3731)

* Add ExpressRoute gateway resource (#3776)

* Initial version

* Refactor connections.

* Update API version

* Add enum values.

* Addressed all comments.

* Adding support for delegations on a subnet (#3805)

* Adding subnet delegation property on VNET

* Updating subnet/vnet create & get examples to reflect delegation

* Adding support for GetAvailableDelegations operation

* Fixing properties in Swagger & updating examples to reflect fixes

* Add availableDelegation as a subResource

* Remove type from examples

* Further updates to delegation PR

* Additional changes to structure

* Correct issues for SDK generation (#3818)

* Remove x-ms-client-flatten attribute as that results in wrong
descriptiosn during generation of the SDKs.

* Missing descriptions.

* Rename TargetUrls to TargetFqdns (#3739)

* Fix travis errors in examples

* Fixed error examples for P2S cortex resources

* Fix swagger

* Fix Semantic validation error

* network swagger fix

* Fix travis

* Fix travis errors

* Revert version change

* Dummy changes for CI restart

* Swagger changes (#3857)

* Relocated readOnly property

* Readded provisioning state to interfaceEndpoint.json

* Updated networkInterface.json

* Updated serviceEndpointPolicy.json

* Update virtualNetworkTap.json

* Fix semantic issue

* Multiple changes

* More example fixes

* Dummy commit

* Fixed duplicate

* changes to swagger in interface endpoints and subnet delegation (#3860)

* Revert "Trusted Root certificate (#3668)"

This reverts commit 3ff4b52.

* fixed api version in azure firewall example

* Review comments incorporation (#3859)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anuchandy anuchandy anuchandy approved these changes

@ravbhatnagar ravbhatnagar Awaiting requested review from ravbhatnagar

Assignees

@anuchandy anuchandy

Labels
ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review potential-sdk-breaking-change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nking-1 @azuresdkci @AutorestCI @anuchandy @ravbhatnagar @panchagnula