-
Notifications
You must be signed in to change notification settings - Fork 4k
EntraID support for DataSync #28464
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
base: main
Are you sure you want to change the base?
EntraID support for DataSync #28464
Conversation
Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status. |
Thank you for your contribution @ssubramanya! We will review the pull request and get back to you soon. |
/// Gets or sets the Datasync identity type Possible values include: 'None', 'SystemAssigned', 'UserAssigned', 'SystemAssignedUserAssigned' | ||
/// </summary> | ||
[Newtonsoft.Json.JsonProperty(PropertyName = "type")] | ||
public string Type {get; set; } |
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.
Why is string chosen instead of enum?
/// </exception> | ||
public virtual void Validate() | ||
{ | ||
if (this.Type == 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.
Also check that type is one of three supported values?
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 about validating other fields?
|
||
/// <param name="userAssignedIdentities">The resource ids of the user assigned identities to use | ||
/// </param> | ||
public DataSyncParticipantIdentity(string type, System.Guid? tenantId = default(System.Guid?), System.Collections.Generic.IDictionary<string, DataSyncParticipantUserAssignedIdentity> userAssignedIdentities = default(System.Collections.Generic.IDictionary<string, DataSyncParticipantUserAssignedIdentity>)) |
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.
Nit: Line too long. Needs a break
|
||
/// <param name="clientId">The Azure Active Directory client id. | ||
/// </param> | ||
public DataSyncParticipantUserAssignedIdentity(System.Guid? principalId = default(System.Guid?), System.Guid? clientId = default(System.Guid?)) |
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.
Should default values be picked up here? I see a private setters. If these are mandatory data, it may be worth considering not defaulting.
/// Gets or sets sync group authentication information. | ||
/// </summary> | ||
[Newtonsoft.Json.JsonProperty(PropertyName = "identity")] | ||
public DataSyncParticipantIdentity Identity {get; set; } |
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.
Constructor takes identity as a mandatory param. Considering that, is private setter appropriate?
@@ -186,10 +196,18 @@ public SyncGroup() | |||
/// </exception> | |||
public virtual void Validate() | |||
{ | |||
if (this.Identity == 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.
Nit: Shorten with _ = this.Identity ?? throw new Microsoft.Rest.ValidationException(Microsoft.Rest.ValidationRules.CannotBeNull, "Identity");
if (this.Sku != null) | ||
{ | ||
this.Sku.Validate(); | ||
} | ||
if (this.Identity != 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.
Nit: Shorten with this.Identity?.Validate()
@@ -8,7 +8,7 @@ namespace Microsoft.Azure.Management.Sql.Models | |||
using System.Linq; | |||
|
|||
/// <summary> | |||
/// Properties of a sync group. | |||
/// Properties of a sync group with support to MI. |
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.
Nit: with support for MI and SQL auth
{ | ||
using System.Linq; | ||
|
||
public partial class SyncGroupsCreateOrUpdateHeaders |
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.
/// <summary>
/// <Brief summary>
/// </summary>
{ | ||
using System.Linq; | ||
|
||
public partial class SyncGroupsUpdateHeaders |
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.
If we already have SyncGroupsCreateOrUpdateHeaders
why can't we reuse that class for update?
/// Gets or sets sync member authentication information. | ||
/// </summary> | ||
[Newtonsoft.Json.JsonProperty(PropertyName = "identity")] | ||
public DataSyncParticipantIdentity Identity {get; set; } |
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.
Constructor takes identity as a mandatory param. Considering that, is private setter appropriate?
/// </exception> | ||
public virtual void Validate() | ||
{ | ||
if (this.Identity == 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.
Nit: Use ??
operator?
} | ||
if (this.Identity != null) | ||
{ | ||
this.Identity.Validate(); |
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.
Nit: Use ?.
operator?
@ssubramanya please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
Description
Note:
The files under "Sql.Management.Sdk" are generated except readme.md - So Please use that as just reference to review for the API calls if needed. Thanks!
Summary of Changes
Draft PR Notice
This is a draft PR.
Added UAMI Support
New-*
andUpdate-*
cmdlets:HubDatabaseAuthenticationType
MemberDatabaseAuthenticationType
IdentityId
These parameters allow customers to:
Password
orUserAssigned
)Backward Compatibility
Identity = { Type = "None" }
in the background.Validation Logic
Username
andPassword
.IdentityId
when creating new resources.UserAssigned
is specified withoutIdentityId
, cmdlet preserves the existing identity instead of throwing.Help and Documentation
Password
,UserAssigned
)Resource Strings
IdentityId
Testing
Mandatory Checklist
Please choose the target release of Azure PowerShell. (⚠️ Target release is a different concept from API readiness. Please click below links for details.)
Check this box to confirm: I have read the Submitting Changes section of
CONTRIBUTING.md
and reviewed the following information:ChangeLog.md
file(s) appropriatelysrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
.## Upcoming Release
header in the past tense.ChangeLog.md
if no new release is required, such as fixing test case only.