added sdk for neon postgres GA version 2025-03-01 #49211
Conversation
alluri02
requested review from
ArcturusZhang,
ArthurMa1978 and
jsquire
as code owners
|
API change check APIView has identified API level changes in this PR and created following API reviews. |
There was a problem hiding this comment.
Your account lacks the public memberships to the "Azure" and "Microsoft" GitHub organizations required of an internal contributor. Please review the Azure SDK onboarding documentation and use the associated Teams channel for support.
- Azure SDK onboarding (Microsoft internal)
- Azure SDK onboarding assistance (Microsoft internal)
You can verify the state of your account by running the Validate-AzsdkCodeOwner script from the Azure SDK tools repository.
Please also be sure to add yourself to CODEOWNERS for this library, if you will be maintaining it going forward.
There was a problem hiding this comment.
I think this change is blocking the CI of this PR to run.
Please revert all the changes in this file.
| System.Collections.Generic.IEnumerator<Azure.ResourceManager.NeonPostgres.BranchResource> System.Collections.Generic.IEnumerable<Azure.ResourceManager.NeonPostgres.BranchResource>.GetEnumerator() { throw null; } | ||
| System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator() { throw null; } | ||
| } | ||
| public partial class BranchData : Azure.ResourceManager.Models.ResourceData, System.ClientModel.Primitives.IJsonModel<Azure.ResourceManager.NeonPostgres.BranchData>, System.ClientModel.Primitives.IPersistableModel<Azure.ResourceManager.NeonPostgres.BranchData> |
There was a problem hiding this comment.
the name Branch here is too generic - we should be more descriptive.
There was a problem hiding this comment.
Hi @ArcturusZhang , Our team develops integrations for third-party ISV partners in the Azure Marketplace. In most cases, the naming conventions are driven by the partners themselves to ensure consistency and familiarity for end users
| System.Collections.Generic.IEnumerator<Azure.ResourceManager.NeonPostgres.ComputeResource> System.Collections.Generic.IEnumerable<Azure.ResourceManager.NeonPostgres.ComputeResource>.GetEnumerator() { throw null; } | ||
| System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator() { throw null; } | ||
| } | ||
| public partial class ComputeData : Azure.ResourceManager.Models.ResourceData, System.ClientModel.Primitives.IJsonModel<Azure.ResourceManager.NeonPostgres.ComputeData>, System.ClientModel.Primitives.IPersistableModel<Azure.ResourceManager.NeonPostgres.ComputeData> |
There was a problem hiding this comment.
same issue here - the name Compute is too generic, we should be more descriptive on the name of a resource.
| System.Collections.Generic.IEnumerator<Azure.ResourceManager.NeonPostgres.EndpointResource> System.Collections.Generic.IEnumerable<Azure.ResourceManager.NeonPostgres.EndpointResource>.GetEnumerator() { throw null; } | ||
| System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator() { throw null; } | ||
| } | ||
| public partial class EndpointData : Azure.ResourceManager.Models.ResourceData, System.ClientModel.Primitives.IJsonModel<Azure.ResourceManager.NeonPostgres.EndpointData>, System.ClientModel.Primitives.IPersistableModel<Azure.ResourceManager.NeonPostgres.EndpointData> |
There was a problem hiding this comment.
same here, Endpoint is too generic.
| System.Collections.Generic.IEnumerator<Azure.ResourceManager.NeonPostgres.ProjectResource> System.Collections.Generic.IEnumerable<Azure.ResourceManager.NeonPostgres.ProjectResource>.GetEnumerator() { throw null; } | ||
| System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator() { throw null; } | ||
| } | ||
| public partial class ProjectData : Azure.ResourceManager.Models.ResourceData, System.ClientModel.Primitives.IJsonModel<Azure.ResourceManager.NeonPostgres.ProjectData>, System.ClientModel.Primitives.IPersistableModel<Azure.ResourceManager.NeonPostgres.ProjectData> |
There was a problem hiding this comment.
same here, Project is too generic.
| } | ||
| public partial class MockableNeonPostgresResourceGroupResource : Azure.ResourceManager.ArmResource | ||
| { | ||
| protected MockableNeonPostgresResourceGroupResource() { } | ||
| public virtual Azure.Response<Azure.ResourceManager.NeonPostgres.NeonOrganizationResource> GetNeonOrganization(string organizationName, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; } | ||
| public virtual System.Threading.Tasks.Task<Azure.Response<Azure.ResourceManager.NeonPostgres.NeonOrganizationResource>> GetNeonOrganizationAsync(string organizationName, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; } | ||
| public virtual Azure.ResourceManager.NeonPostgres.NeonOrganizationCollection GetNeonOrganizations() { throw null; } | ||
| public virtual Azure.Response<Azure.ResourceManager.NeonPostgres.Models.PgVersionsResult> GetPostgresVersionsOrganization(Azure.ResourceManager.NeonPostgres.Models.PgVersion pgVersion = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; } |
There was a problem hiding this comment.
please also fix the name of this operation - I think it should be GetOrganizationPostgresVersions
| public static Azure.ResourceManager.NeonPostgres.ProjectData ProjectData(Azure.Core.ResourceIdentifier id = null, string name = null, Azure.Core.ResourceType resourceType = default(Azure.Core.ResourceType), Azure.ResourceManager.Models.SystemData systemData = null, Azure.ResourceManager.NeonPostgres.Models.ProjectProperties properties = null) { throw null; } | ||
| public static Azure.ResourceManager.NeonPostgres.Models.ProjectProperties ProjectProperties(string entityId = null, string entityName = null, string createdAt = null, Azure.ResourceManager.NeonPostgres.Models.NeonResourceProvisioningState? provisioningState = default(Azure.ResourceManager.NeonPostgres.Models.NeonResourceProvisioningState?), System.Collections.Generic.IEnumerable<Azure.ResourceManager.NeonPostgres.Models.Attributes> attributes = null, string regionId = null, long? storage = default(long?), int? pgVersion = default(int?), int? historyRetention = default(int?), Azure.ResourceManager.NeonPostgres.Models.DefaultEndpointSettings defaultEndpointSettings = null, Azure.ResourceManager.NeonPostgres.Models.BranchProperties branch = null, System.Collections.Generic.IEnumerable<Azure.ResourceManager.NeonPostgres.Models.NeonRoleProperties> roles = null, System.Collections.Generic.IEnumerable<Azure.ResourceManager.NeonPostgres.Models.NeonDatabaseProperties> databases = null, System.Collections.Generic.IEnumerable<Azure.ResourceManager.NeonPostgres.Models.EndpointProperties> endpoints = null) { throw null; } | ||
| } | ||
| public partial class Attributes : System.ClientModel.Primitives.IJsonModel<Azure.ResourceManager.NeonPostgres.Models.Attributes>, System.ClientModel.Primitives.IPersistableModel<Azure.ResourceManager.NeonPostgres.Models.Attributes> |
There was a problem hiding this comment.
Single word name is not allowed, we need a more descriptive name for this class.
There was a problem hiding this comment.
Besides, based on the properties of this type, should the name here be singular?
It just has Name and Value, why it is a "attributes" instead of "attribute"?
| string System.ClientModel.Primitives.IPersistableModel<Azure.ResourceManager.NeonPostgres.Models.Attributes>.GetFormatFromOptions(System.ClientModel.Primitives.ModelReaderWriterOptions options) { throw null; } | ||
| System.BinaryData System.ClientModel.Primitives.IPersistableModel<Azure.ResourceManager.NeonPostgres.Models.Attributes>.Write(System.ClientModel.Primitives.ModelReaderWriterOptions options) { throw null; } | ||
| } | ||
| public partial class BranchProperties : System.ClientModel.Primitives.IJsonModel<Azure.ResourceManager.NeonPostgres.Models.BranchProperties>, System.ClientModel.Primitives.IPersistableModel<Azure.ResourceManager.NeonPostgres.Models.BranchProperties> |
There was a problem hiding this comment.
this name should change in the same pattern of Branch.
| public string EntityId { get { throw null; } } | ||
| public string EntityName { get { throw null; } set { } } | ||
| public string ParentId { get { throw null; } set { } } | ||
| public string ProjectId { get { throw null; } set { } } |
There was a problem hiding this comment.
Would these XXXId properties be "arm-id"s?
If so, I think we should modify our typespec to use armResourceIdentifier.
Please do not worry if this would impact other SDKs - only dotnet would generate something else for armResourceIdentifier type, other languages would still handle it as a string.
| string System.ClientModel.Primitives.IPersistableModel<Azure.ResourceManager.NeonPostgres.Models.BranchProperties>.GetFormatFromOptions(System.ClientModel.Primitives.ModelReaderWriterOptions options) { throw null; } | ||
| System.BinaryData System.ClientModel.Primitives.IPersistableModel<Azure.ResourceManager.NeonPostgres.Models.BranchProperties>.Write(System.ClientModel.Primitives.ModelReaderWriterOptions options) { throw null; } | ||
| } | ||
| public partial class ComputeProperties : System.ClientModel.Primitives.IJsonModel<Azure.ResourceManager.NeonPostgres.Models.ComputeProperties>, System.ClientModel.Primitives.IPersistableModel<Azure.ResourceManager.NeonPostgres.Models.ComputeProperties> |
There was a problem hiding this comment.
this name should change following the change for model Compute.
| string System.ClientModel.Primitives.IPersistableModel<Azure.ResourceManager.NeonPostgres.Models.DefaultEndpointSettings>.GetFormatFromOptions(System.ClientModel.Primitives.ModelReaderWriterOptions options) { throw null; } | ||
| System.BinaryData System.ClientModel.Primitives.IPersistableModel<Azure.ResourceManager.NeonPostgres.Models.DefaultEndpointSettings>.Write(System.ClientModel.Primitives.ModelReaderWriterOptions options) { throw null; } | ||
| } | ||
| public partial class EndpointProperties : System.ClientModel.Primitives.IJsonModel<Azure.ResourceManager.NeonPostgres.Models.EndpointProperties>, System.ClientModel.Primitives.IPersistableModel<Azure.ResourceManager.NeonPostgres.Models.EndpointProperties> |
There was a problem hiding this comment.
this name should change in the same pattern of Endpoint.
| System.BinaryData System.ClientModel.Primitives.IPersistableModel<Azure.ResourceManager.NeonPostgres.Models.EndpointProperties>.Write(System.ClientModel.Primitives.ModelReaderWriterOptions options) { throw null; } | ||
| } | ||
| [System.Runtime.InteropServices.StructLayoutAttribute(System.Runtime.InteropServices.LayoutKind.Sequential)] | ||
| public readonly partial struct EndpointType : System.IEquatable<Azure.ResourceManager.NeonPostgres.Models.EndpointType> |
There was a problem hiding this comment.
this type should also follow the name change for Endpoint.
| public string BranchId { get { throw null; } set { } } | ||
| public string CreatedAt { get { throw null; } } | ||
| public string EntityId { get { throw null; } } |
There was a problem hiding this comment.
would the XXXId properties be arm-ids?
If so, we should change their types in our specs to armResourceIdentifier.
| public string BranchId { get { throw null; } set { } } | ||
| public string CreatedAt { get { throw null; } } | ||
| public string EntityId { get { throw null; } } |
There was a problem hiding this comment.
same question here for BranchId and EntityId.
| public NeonRoleProperties() { } | ||
| public System.Collections.Generic.IList<Azure.ResourceManager.NeonPostgres.Models.Attributes> Attributes { get { throw null; } } | ||
| public string BranchId { get { throw null; } set { } } | ||
| public string CreatedAt { get { throw null; } } |
There was a problem hiding this comment.
would this be a utc time or a date time?
if so, I think we should change it in our spec to let it be a date-time of your format - but this could be a breaking change to your spec and to other languages' SDK.
| @@ -251,4 +737,50 @@ protected virtual void JsonModelWriteCore(System.Text.Json.Utf8JsonWriter writer | |||
| string System.ClientModel.Primitives.IPersistableModel<Azure.ResourceManager.NeonPostgres.Models.PartnerOrganizationProperties>.GetFormatFromOptions(System.ClientModel.Primitives.ModelReaderWriterOptions options) { throw null; } | |||
| System.BinaryData System.ClientModel.Primitives.IPersistableModel<Azure.ResourceManager.NeonPostgres.Models.PartnerOrganizationProperties>.Write(System.ClientModel.Primitives.ModelReaderWriterOptions options) { throw null; } | |||
| } | |||
| public partial class PgVersion : System.ClientModel.Primitives.IJsonModel<Azure.ResourceManager.NeonPostgres.Models.PgVersion>, System.ClientModel.Primitives.IPersistableModel<Azure.ResourceManager.NeonPostgres.Models.PgVersion> | |||
There was a problem hiding this comment.
I think PgVersion is quite confusing - could we expand the abbr Pg here?
| string System.ClientModel.Primitives.IPersistableModel<Azure.ResourceManager.NeonPostgres.Models.PgVersion>.GetFormatFromOptions(System.ClientModel.Primitives.ModelReaderWriterOptions options) { throw null; } | ||
| System.BinaryData System.ClientModel.Primitives.IPersistableModel<Azure.ResourceManager.NeonPostgres.Models.PgVersion>.Write(System.ClientModel.Primitives.ModelReaderWriterOptions options) { throw null; } | ||
| } | ||
| public partial class PgVersionsResult : System.ClientModel.Primitives.IJsonModel<Azure.ResourceManager.NeonPostgres.Models.PgVersionsResult>, System.ClientModel.Primitives.IPersistableModel<Azure.ResourceManager.NeonPostgres.Models.PgVersionsResult> |
| string System.ClientModel.Primitives.IPersistableModel<Azure.ResourceManager.NeonPostgres.Models.PgVersionsResult>.GetFormatFromOptions(System.ClientModel.Primitives.ModelReaderWriterOptions options) { throw null; } | ||
| System.BinaryData System.ClientModel.Primitives.IPersistableModel<Azure.ResourceManager.NeonPostgres.Models.PgVersionsResult>.Write(System.ClientModel.Primitives.ModelReaderWriterOptions options) { throw null; } | ||
| } | ||
| public partial class ProjectProperties : System.ClientModel.Primitives.IJsonModel<Azure.ResourceManager.NeonPostgres.Models.ProjectProperties>, System.ClientModel.Primitives.IPersistableModel<Azure.ResourceManager.NeonPostgres.Models.ProjectProperties> |
There was a problem hiding this comment.
this name should change following the same pattern for type Project.
| public partial class ProjectProperties : System.ClientModel.Primitives.IJsonModel<Azure.ResourceManager.NeonPostgres.Models.ProjectProperties>, System.ClientModel.Primitives.IPersistableModel<Azure.ResourceManager.NeonPostgres.Models.ProjectProperties> | ||
| { | ||
| public ProjectProperties() { } | ||
| public System.Collections.Generic.IList<Azure.ResourceManager.NeonPostgres.Models.Attributes> Attributes { get { throw null; } } |
There was a problem hiding this comment.
as I said above - now we have a list of "attributes", and the property is called attributes.
If it could make more sense, it should be singular for the type Attributes because it only represents one instance.
| public System.Collections.Generic.IList<Azure.ResourceManager.NeonPostgres.Models.NeonDatabaseProperties> Databases { get { throw null; } } | ||
| public Azure.ResourceManager.NeonPostgres.Models.DefaultEndpointSettings DefaultEndpointSettings { get { throw null; } set { } } | ||
| public System.Collections.Generic.IList<Azure.ResourceManager.NeonPostgres.Models.EndpointProperties> Endpoints { get { throw null; } } | ||
| public string EntityId { get { throw null; } } |
There was a problem hiding this comment.
would this be an arm-id? if so we should change its type in your typespec to be armResourceIdentifier.
| public int? HistoryRetention { get { throw null; } set { } } | ||
| public int? PgVersion { get { throw null; } set { } } | ||
| public Azure.ResourceManager.NeonPostgres.Models.NeonResourceProvisioningState? ProvisioningState { get { throw null; } } | ||
| public string RegionId { get { throw null; } set { } } |
There was a problem hiding this comment.
would this be an arm-id? if so we should change its type in your typespec to be armResourceIdentifier.
There was a problem hiding this comment.
a couple of comments on names.
To resolve them, please open a PR in azure-rest-api-specs, and add
@@clientName(the.property, "newName", "csharp");
or if you think these suggestions make sense, you could directly change them in your spec, and then change the commit id here in tsp-location.yaml file to be the head commit of your PR and regenerate the code.
Please also paste the link of your swagger PR in this PR, we would review two PRs in the same time.
Hi, forked the repository and creating a new PR as per this - https://github.com/Azure/azure-sdk-for-net/blob/main/CONTRIBUTING.md |
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.