Skip to content

Conversation

@dingmeng-xue
Copy link
Member

Description

Replace Sql management client using generic rest client

Checklist

  • I have read the Submitting Changes section of CONTRIBUTING.md
  • The title of the PR is clear and informative
  • The appropriate ChangeLog.md file(s) has been updated:
    • For any service, the ChangeLog.md file can be found at src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
    • A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header -- no new version header should be added
  • The PR does not introduce breaking changes
  • If applicable, the changes made in the PR have proper test coverage
  • For public API changes to cmdlets:
    • a cmdlet design review was approved for the changes in this repository (Microsoft internal only)
    • the markdown help files have been regenerated using the commands listed here

@dingmeng-xue dingmeng-xue added this to the S167 (2020-03-31) milestone Mar 21, 2020
@adxsdkps
Copy link
Collaborator

Can one of the admins verify this patch?

@dingmeng-xue dingmeng-xue marked this pull request as ready for review March 21, 2020 15:04
@erich-wang erich-wang self-assigned this Mar 23, 2020
erich-wang
erich-wang previously approved these changes Mar 23, 2020
else
{
IPage<PrivateLinkService> plsPage;
IPage<Microsoft.Azure.Management.Network.Models.PrivateLinkService> plsPage;
Copy link
Member

Choose a reason for hiding this comment

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

nit: suggest to use using namespace

Copy link
Member Author

Choose a reason for hiding this comment

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

using this one now.
using NM = Microsoft.Azure.Management.Network.Models;

/// the private endpoint connection.</param>
/// <param name="provisioningState">State of the private endpoint
/// connection.</param>
public PrivateEndpointConnection(string id = default(string), string name = default(string), string type = default(string), PrivateEndpointProperty privateEndpoint = default(PrivateEndpointProperty), PrivateLinkServiceConnectionStateProperty privateLinkServiceConnectionState = default(PrivateLinkServiceConnectionStateProperty), string provisioningState = default(string))
Copy link
Member

Choose a reason for hiding this comment

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

nit: usually we use null as default value for reference object instead of default(xxx), I guess this is generated code, so should be fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

copied from generated code.


namespace Microsoft.Azure.Commands.Network.PrivateLinkService.PrivateLinkServiceProvider
{
public class PrivateLinkProviderFactory
Copy link
Member

Choose a reason for hiding this comment

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

mark this class as internal?

switch (privateLinkResourceType.ToLower())
{
case NETWORKING_TYPE:
default:
Copy link
Member

Choose a reason for hiding this comment

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

for unknown type, should we return error or throw exceptions?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's network original logic. If resource type is unknown, returns NetworkProvider.


namespace Microsoft.Azure.Commands.Network.PrivateLinkService.PrivateLinkServiceProvider
{
public class GenericProvider : IPrivateLinkProvider
Copy link
Member

Choose a reason for hiding this comment

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

mark as internal?

@erich-wang erich-wang merged commit a29269d into Azure:master Mar 23, 2020
dingmeng-xue pushed a commit to dingmeng-xue/azure-powershell that referenced this pull request Mar 29, 2020
Replace Sql management client using generic rest client
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