Skip to content

Regenerated WebSites SDK for January 2017 release#2763

Merged
cormacpayne merged 2 commits intoAzure:AutoRestfrom
nking-1:AutoRest
Feb 3, 2017
Merged

Regenerated WebSites SDK for January 2017 release#2763
cormacpayne merged 2 commits intoAzure:AutoRestfrom
nking-1:AutoRest

Conversation

@nking-1
Copy link
Copy Markdown

@nking-1 nking-1 commented Jan 27, 2017

Description

Regenerated the WebSites SDK from Swagger spec. Link to Swagger PR:
Azure/azure-rest-api-specs#878
Azure/azure-rest-api-specs#881

The SDK version has been bumped because of potential breaking changes.


This checklist is used to make sure that common guidelines for a pull request are followed.

General Guidelines

  • Title of the pull request 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 more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

SDK Generation Guidelines

  • If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
  • The generate.cmd file for the SDK has been updated with the version of AutoRest, as well as a link to the swagger spec, used to generate the code.
  • The project.json and AssemblyInfo.cs files have been updated with the new version of the SDK.

@azurecla
Copy link
Copy Markdown

Hi @Nking92, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!


It looks like you're working at Microsoft (nicking). If you're full-time, we DON'T require a contribution license agreement.



If you are a vendor, DO please sign the electronic contribution license agreement. It will take 2 minutes and there's no faxing! https://cla.azure.com.

TTYL, AZPRBOT;

Copy link
Copy Markdown
Member

@cormacpayne cormacpayne left a comment

Choose a reason for hiding this comment

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

@Nking92 Hey Nick, I left a dozen or so comments.

  1. The comments about "removal of default parameters" or "ordering has changed for parameters in the constructor" applies to a lot of the model classes, so I stopped leaving the comment after the first few files so it didn't litter the review.

  2. Since the Websites SDK is still in preview, these breaking changes should be fine to accept, but you will need to mitigate these changes if you do decide to make any changes in PowerShell

/// <param name="postalCode">Postal code.</param>
/// <param name="state">State.</param>
/// <param name="address2">Address 2.</param>
public Address(string address1, string city, string country, string postalCode, string state, string address2 = default(string))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Nking92 breaking change - some default values were removed, and the ordering of the parameters have changed

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

We changed some of these parameters to be required since our REST API actually rejects null values here

/// 'AzureServiceUnauthorizedToAccessKeyVault', 'KeyVaultDoesNotExist',
/// 'KeyVaultSecretDoesNotExist', 'UnknownError', 'ExternalPrivateKey',
/// 'Unknown'</param>
public AppServiceCertificate(string location, string id = default(string), string name = default(string), string kind = default(string), string type = default(string), IDictionary<string, string> tags = default(IDictionary<string, string>), string keyVaultId = default(string), string keyVaultSecretName = default(string), KeyVaultSecretStatus? provisioningState = default(KeyVaultSecretStatus?))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Nking92 breaking change - the ordering of the parameters has changed

/// <param name="isPrivateKeyExternal">&lt;code&gt;true&lt;/code&gt; if
/// private key is external; otherwise,
/// &lt;code&gt;false&lt;/code&gt;.</param>
public AppServiceCertificateOrder(string location, string id = default(string), string name = default(string), string kind = default(string), string type = default(string), IDictionary<string, string> tags = default(IDictionary<string, string>), IDictionary<string, AppServiceCertificate> certificates = default(IDictionary<string, AppServiceCertificate>), string distinguishedName = default(string), string domainVerificationToken = default(string), int? validityInYears = default(int?), int? keySize = default(int?), CertificateProductType? productType = default(CertificateProductType?), bool? autoRenew = default(bool?), ProvisioningState? provisioningState = default(ProvisioningState?), CertificateOrderStatus? status = default(CertificateOrderStatus?), CertificateDetails signedCertificate = default(CertificateDetails), string csr = default(string), CertificateDetails intermediate = default(CertificateDetails), CertificateDetails root = default(CertificateDetails), string serialNumber = default(string), System.DateTime? lastCertificateIssuanceTime = default(System.DateTime?), System.DateTime? expirationTime = default(System.DateTime?), bool? isPrivateKeyExternal = default(bool?))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Nking92 breaking change - ordering of parameters has changed

@@ -1,72 +0,0 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Nking92 breaking change - removal of the entire file

/// </summary>
[JsonProperty(PropertyName = "frequencyInterval")]
public int? FrequencyInterval { get; set; }
public int FrequencyInterval { get; set; }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Nking92 breaking change - null is no longer an accepted value for this property

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This is a required property, so we'll add a default non-null value here

/// On/off flag indicating the rule is currently enabled or disabled.
/// </summary>
[JsonProperty(PropertyName = "enabled")]
public int? Enabled { get; set; }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Nking92 breaking change - removal of property

/// </summary>
[JsonProperty(PropertyName = "httpApiPrefixPath")]
public string HttpApiPrefixPath { get; set; }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Nking92 breaking change - removal of property

/// <summary>
/// </summary>
[JsonProperty(PropertyName = "openIdIssuer")]
public string OpenIdIssuer { get; set; }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Nking92 same comment for AadClientId and OpenIdIssuer

@@ -1,66 +0,0 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Nking92 breaking change - removal of file

/// </summary>
[JsonProperty(PropertyName = "properties")]
public ValidateProperties Properties { get; set; }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Nking92 breaking change - removal of property

@nking-1
Copy link
Copy Markdown
Author

nking-1 commented Jan 30, 2017

After discussing the review comments with @naveedaz we've decided to keep the breaking changes.

The BackupSchedule properties that were previously nullable will have default values added soon. I've filed an issue for it. We'd like to not block this SDK update on fixing that small bug, and instead fix it in a smaller PR.

@nking-1 nking-1 requested a review from markcowl February 1, 2017 22:39
@cormacpayne
Copy link
Copy Markdown
Member

LGTM

@cormacpayne cormacpayne merged commit dbbf097 into Azure:AutoRest Feb 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants