-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[PostgreSQL] SDK upgrade for PostgreSQL flexible server to version 2022-03-08-privatepreview #31641
[PostgreSQL] SDK upgrade for PostgreSQL flexible server to version 2022-03-08-privatepreview #31641
Conversation
...ql/Azure.ResourceManager.PostgreSql/src/PostgreSqlFlexibleServers/Custom/FastProvisioning.cs
Outdated
Show resolved
Hide resolved
sdk/postgresql/Azure.ResourceManager.PostgreSql/tests/Scenario/PostgreSqlFlexibleServerTests.cs
Show resolved
Hide resolved
/// <param name="waitUntil"> <see cref="WaitUntil.Completed"/> if the method should wait to return until the long-running operation has completed on the service; <see cref="WaitUntil.Started"/> if it should return after starting the operation. For more information on long-running operations, please see <see href="https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/core/Azure.Core/samples/LongRunningOperations.md"> Azure.Core Long-Running Operation samples</see>. </param> | ||
/// <param name="data"> The required parameters for creating a server. </param> | ||
/// <param name="cancellationToken"> The cancellation token to use. </param> | ||
public virtual ArmOperation<PostgreSqlFlexibleServerResource> FastProvision(WaitUntil waitUntil, PostgreSqlFlexibleServerData data, CancellationToken cancellationToken = default) |
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.
What will be the example of using this method like? Can we possibly add a sample of this in the sample repo if possible?
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.
Sure, after this gets merged, I'll add a sample.
...stgresql/Azure.ResourceManager.PostgreSql/src/PostgreSqlFlexibleServers/Custom/FastCreate.cs
Show resolved
Hide resolved
@@ -426,6 +426,8 @@ public static partial class FlexibleServersExtensions | |||
{ | |||
public static Azure.Response<Azure.ResourceManager.PostgreSql.FlexibleServers.Models.PostgreSqlFlexibleServerNameAvailabilityResult> CheckPostgreSqlFlexibleServerNameAvailability(this Azure.ResourceManager.Resources.SubscriptionResource subscriptionResource, Azure.ResourceManager.PostgreSql.FlexibleServers.Models.PostgreSqlFlexibleServerNameAvailabilityContent content, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; } | |||
public static System.Threading.Tasks.Task<Azure.Response<Azure.ResourceManager.PostgreSql.FlexibleServers.Models.PostgreSqlFlexibleServerNameAvailabilityResult>> CheckPostgreSqlFlexibleServerNameAvailabilityAsync(this Azure.ResourceManager.Resources.SubscriptionResource subscriptionResource, Azure.ResourceManager.PostgreSql.FlexibleServers.Models.PostgreSqlFlexibleServerNameAvailabilityContent content, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; } | |||
public static Azure.Response<Azure.ResourceManager.PostgreSql.FlexibleServers.Models.PostgreSqlFlexibleServerCachedServerName> ExecuteGetCachedServerName(this Azure.ResourceManager.Resources.ResourceGroupResource resourceGroupResource, Azure.Core.AzureLocation locationName, Azure.ResourceManager.PostgreSql.FlexibleServers.Models.PostgreSqlFlexibleServerCachedServerNameContent content, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; } |
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 word Execute
looks a bit redundant, can we rename this operation to simply GetCachedServerName
?
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.
Done, I renamed it to GetPostgreSqlFlexibleServerCachedServerName
to have consistency with other operations.
/// <param name="status"> The status. </param> | ||
internal PostgreSqlFlexibleServerCapabilityProperties(string zone, bool? isGeoBackupSupported, bool? isZoneRedundantHASupported, bool? isZoneRedundantHAAndGeoBackupSupported, IReadOnlyList<PostgreSqlFlexibleServerEditionCapability> supportedFlexibleServerEditions, IReadOnlyList<PostgreSqlFlexibleServerHyperscaleNodeEditionCapability> supportedHyperscaleNodeEditions, IReadOnlyList<string> supportedHAModes, string status) |
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 is a breaking change. I saw the new version is 1.1.0-beta1
, so it is OK. But you only change the sequence of the parameter, which should be avoided.
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.
@alanenriqueo I just realized that there is already an official release 1.0.0
. So this is a breaking change which needs to be resolved.
Here are 2 options:
- Add manual codes to provide backward compatibility, see compute for reference: https://github.com/Azure/azure-sdk-for-net/tree/main/sdk/compute/Microsoft.Azure.Management.Compute/src/Customizations
- Or you need to bump the major version, e.g.
2.0.0
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.
Changing order of properties is no longer a brekaing change in track 2 SDKs
...rs/Generated/Models/PostgreSqlFlexibleServerVirtualNetworkSubnetUsageResult.Serialization.cs
Show resolved
Hide resolved
@@ -624,6 +632,36 @@ public partial class PostgreSqlFlexibleServerResource : Azure.ResourceManager.Ar | |||
public virtual Azure.ResourceManager.ArmOperation<Azure.ResourceManager.PostgreSql.FlexibleServers.PostgreSqlFlexibleServerResource> Update(Azure.WaitUntil waitUntil, Azure.ResourceManager.PostgreSql.FlexibleServers.Models.PostgreSqlFlexibleServerPatch patch, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; } | |||
public virtual System.Threading.Tasks.Task<Azure.ResourceManager.ArmOperation<Azure.ResourceManager.PostgreSql.FlexibleServers.PostgreSqlFlexibleServerResource>> UpdateAsync(Azure.WaitUntil waitUntil, Azure.ResourceManager.PostgreSql.FlexibleServers.Models.PostgreSqlFlexibleServerPatch patch, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; } | |||
} | |||
public partial class PostgreSqlFlexibleServerServerBackupCollection : Azure.ResourceManager.ArmCollection, System.Collections.Generic.IAsyncEnumerable<Azure.ResourceManager.PostgreSql.FlexibleServers.PostgreSqlFlexibleServerServerBackupResource>, System.Collections.Generic.IEnumerable<Azure.ResourceManager.PostgreSql.FlexibleServers.PostgreSqlFlexibleServerServerBackupResource>, System.Collections.IEnumerable |
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.
From the SDK API perspective, this isn't a good resource name. Why do we have two consecutive word Server
? Can we omit one of them?
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.
Done.
639f00c
to
4ec2526
Compare
4ec2526
to
42dbf73
Compare
@@ -14,13 +14,13 @@ modelerfour: | |||
|
|||
batch: | |||
- tag: package-2020-01-01 | |||
- tag: package-flexibleserver-2021-06 | |||
- tag: flexibleserver-2022-03-privatepreview |
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 Azure/azure-rest-api-specs#20212 was merged 20 mins ago, do you need to update the sdk accordingly?
@@ -1,14 +1,11 @@ | |||
# Release History | |||
|
|||
## 1.1.0-beta.1 (Unreleased) | |||
## 1.1.0-beta.1 (2022-10-11) |
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 update this to a new release day as the swagger has just been merged 20 mins ago
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.
And please also flip the pr from draft
to Ready for review
Hi @alanenriqueo. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
Hi @alanenriqueo. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing "/reopen" if you'd like to continue working on these changes. Please be sure to use the command to reopen or remove the "no-recent-activity" label; otherwise, this is likely to be closed again with the next cleanup pass. |
PostgreSqlFlexibleServers
.FastCreate
custom function for fast provisioning feature.Contributing to the Azure SDK
Please see our CONTRIBUTING.md if you are not familiar with contributing to this repository or have questions.
For specific information about pull request etiquette and best practices, see this section.