Resources improvements #6008

cormacpayne · 2018-04-20T16:30:39Z

Description

Fix for issue #5903

Update AD cmdlets so they conform with PowerShell standards
Add new parameter set to New-AzureRmADServicePrincipal for simple creation of a service principal
Remove Find-AzureRmResource and update Get-AzureRmResource to work in basic scenarios
Remove Find-AzureRmResourceGroup and move missing functionality into Get-AzureRmResourceGroup

Checklist

I have read the Submitting Changes section of CONTRIBUTING.md
The title of the PR is clear and informative
The appropriate change log has been updated
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:
- the changes have gone through a cmdlet design review
- the cmdlet markdown files were generated using the platyPS module

…ions

markcowl

Partial PR review, will complete

markcowl · 2018-04-23T03:07:42Z

src/Common/Commands.Common.Graph.RBAC/ActiveDirectory/ActiveDirectoryClient.cs

-            };
-
-            PSADServicePrincipal servicePrincipal = FilterServicePrincipals(options).FirstOrDefault();
+            var objId = objectId.ToString();


better to use a descriptive identifier, like objectIdString

markcowl · 2018-04-23T03:10:22Z

...ceManager/Common/Commands.ResourceManager.Common/ArgumentCompleters/ResourceTypeCompleter.cs

+    using System.Management.Automation;
+
+    /// <summary>
+    /// This attribute will allow the user to autocomplete the -ResourceType parameter of a cmdlet with valid resource types


Can we split this into a separate PR? I want to make sure we get in as much of this as possible

markcowl · 2018-04-23T03:14:44Z

...ceManager/Common/Commands.ResourceManager.Common/ArgumentCompleters/ResourceTypeCompleter.cs

+                                instance.ClientFactory.GetCustomHandlers());
+                            client.SubscriptionId = context.Subscription.Id;
+                            // Retrieve only the first page of resource types to display to the user
+                            var resourceTypes = client.Providers.ListAsync();


why only one page?

@markcowl looks like this was a copy-paste issue -- I changed this so it retrieves all pages

markcowl · 2018-04-23T03:17:27Z

src/ResourceManager/Common/Commands.ResourceManager.Common/ArgumentCompleters/ScopeCompleter.cs

+
+                        catch (Exception ex)
+                        {
+                            if (ex == null) { }


I think this check should also be in the ifdef

@markcowl this was modeled after the ResourceGroupCompleter, but I believe that it's here so that the release build won't fail due to an unused ex variable

markcowl · 2018-04-23T03:19:11Z

src/ResourceManager/Common/Commands.ResourceManager.Common/ArgumentCompleters/ScopeCompleter.cs

+                                instance.AuthenticationFactory.GetServiceClientCredentials(context, AzureEnvironment.Endpoint.ResourceManager),
+                                instance.ClientFactory.GetCustomHandlers());
+                            client.SubscriptionId = context.Subscription.Id;
+                            // Retrieve only the first page of ResourceGroups to display to the user


I wonder if we can't pass in the parameter value through to the resource groups call to make the first page of results more effective

markcowl · 2018-04-23T03:21:28Z

src/ResourceManager/Common/Commands.ResourceManager.Common/Paging/PageEnumerable.cs

@@ -19,44 +19,31 @@
 using System.Collections.Generic;
 using System.Threading;

-namespace Microsoft.Azure.Internal.Subscriptions.Models
+namespace Microsoft.Azure.Commands.ResourceManager.Common.Paging


same comment - we cannot make breaking changes here. If we need to substantially redesign it, we should define a new class.

@markcowl this has been reverted

markcowl · 2018-04-23T03:21:45Z

src/ResourceManager/Common/Commands.ResourceManager.Common/Paging/PageEnumerator.cs

@@ -17,23 +17,36 @@
 using System.Collections.Generic;
 using System.Collections;

-namespace Microsoft.Azure.Internal.Subscriptions.Models


same comment

@markcowl this has been reverted

markcowl · 2018-04-23T03:22:36Z

src/ResourceManager/KeyVault/Commands.KeyVault/Models/ModelExtensions.cs

@@ -113,7 +113,8 @@ public static string GetDisplayNameForADObject(string objectId, ActiveDirectoryC
                    }
                    else if (obj.Type.Equals("serviceprincipal", StringComparison.InvariantCultureIgnoreCase))
                    {
-                        var servicePrincipal = adClient.FilterServicePrincipals(new ADObjectFilterOptions { Id = objectId }).FirstOrDefault();
+                        var odataQuery = new Rest.Azure.OData.ODataQuery<Graph.RBAC.Version1_6.Models.ServicePrincipal>(s => s.ObjectId == objectId);


we should let the keyvault team look at these changes

markcowl · 2018-04-23T03:29:02Z

...urceManager/Resources/Commands.ResourceManager/Cmdlets/SdkClient/ResourceManagerSdkClient.cs

@@ -607,6 +608,28 @@ public virtual List<PSResourceGroup> FilterResourceGroups(string name, Hashtable
                    resourceGroups.AddRange(listResult);
                }

+                if (!string.IsNullOrEmpty(name))


ToDo: This would be really nice extracted out into a helper like:

IEnumerable<T> FilterByWildCard<T>(IEnumerable<T> source, Func<T, string> getKey, string parameterValue)

markcowl · 2018-04-23T03:30:47Z

...ResourceManager/Resources/Commands.ResourceManager/Cmdlets/SdkModels/Resources/PSResource.cs

+{
+    public class PSResource
+    {
+        public string Id { get; set; }


Need ResourceId for backward-compatible cmdlet piping, right?

markcowl · 2018-04-23T03:56:02Z

@maddieclayton Can we get someone from KeyVaule to pass on the small changes here to KeyVault cmdlets?

markcowl

ALso, we need a plan for test coverage

markcowl · 2018-04-23T16:40:58Z

...ResourceManager/Resources/Commands.Resources/ActiveDirectory/AddAzureADGroupMemberCommand.cs

+        [Parameter(Mandatory = true, ValueFromPipelineByPropertyName = true, ParameterSetName = ParameterSet.Explicit, HelpMessage = "The object id of the member.")]
+        [Parameter(Mandatory = true, ParameterSetName = ParameterSet.GroupObject, HelpMessage = "The object id of the member.")]
+        [ValidateNotNullOrEmpty]
+        public Guid MemberObjectId { get; set; }


For this to be parallel with other cmdlets using AD, we would want to have parameters to allow users to specify thee target group by name, and the user principals.

Also, for each of the parameters specifying things to add to the target group, including memberObjectId, I would make the parameter type an array, so we cna add multiple.

markcowl · 2018-04-23T16:41:42Z

...ResourceManager/Resources/Commands.Resources/ActiveDirectory/AddAzureADGroupMemberCommand.cs

+
+        [Parameter(Mandatory = true, ValueFromPipelineByPropertyName = true, ParameterSetName = ParameterSet.Explicit, HelpMessage = "The object id of the group to add the member to.")]
+        [ValidateNotNullOrEmpty]
+        public Guid GroupObjectId { get; set; }


I would change this to TargetGroupObjectId, and TargetGroupObject

markcowl · 2018-04-23T16:45:35Z

...ResourceManager/Resources/Commands.Resources/ActiveDirectory/GetAzureADGroupMemberCommand.cs

        [ValidateNotNullOrEmpty]
-        public Guid GroupObjectId { get; set; }
+        [Alias("Id", "GroupObjectId")]


We should consider having a group name parameter here, if it does not exist already

markcowl · 2018-04-23T16:47:31Z

...esourceManager/Resources/Commands.Resources/ActiveDirectory/GetAzureADSpCredentialCommand.cs

@@ -21,21 +23,32 @@ namespace Microsoft.Azure.Commands.ActiveDirectory
    /// Gets AD service principal credentials.
    /// </summary>
    [Cmdlet(VerbsCommon.Get, "AzureRmADSpCredential", DefaultParameterSetName = ParameterSet.ObjectId), OutputType(typeof(PSADCredential))]
+    [Alias("Get-AzureRmADServicePrincipalCredential")]


Get-AzureRmADSpnCredential as well

@markcowl I think we agreed to skip this comment (I can add the alias if that's not the case)

markcowl · 2018-04-23T16:49:28Z

src/ResourceManager/Resources/Commands.Resources/ActiveDirectory/GetAzureADUserCommand.cs

+                    SearchString = SearchString,
+                    UPN = UserPrincipalName,
+                    Id = ObjectId == Guid.Empty ? null : ObjectId.ToString(),
+                    Paging = true,


what does the Paging option do here?

@markcowl it used to signal that the client should follow the nextPageLink, but that is now handled by GenericPageEnumerable, so this isn't necessarily needed (but I will keep it around 😀 )

markcowl · 2018-04-23T16:59:46Z

...Manager/Resources/Commands.Resources/ActiveDirectory/UpdateAzureADServicePrincipalCommand.cs

+        [Parameter(Mandatory = true, ValueFromPipelineByPropertyName = true, ParameterSetName = ParameterSet.SPNWithDisplayName, HelpMessage = "The display name for the application.")]
+        [Parameter(Mandatory = true, ParameterSetName = ParameterSet.InputObjectWithDisplayName, HelpMessage = "The display name for the application")]
+        [ValidateNotNullOrEmpty]
+        public string DisplayName { get; set; }


is this the only thins we can actually change?

@markcowl added Homepage, IdentifierUri, KeyCredential and PasswordCredential parameters

markcowl · 2018-04-23T17:03:08Z

src/ResourceManager/Resources/Commands.Resources/help/Get-AzureRmADUser.md

@@ -81,7 +97,39 @@ Accept pipeline input: False
 Accept wildcard characters: False
 ```

+### -First
+Gets the first N objects.


I would say The maximum number of objects to return

markcowl · 2018-04-23T17:35:41Z

src/ResourceManager/Resources/Commands.Resources/help/Update-AzureRmADApplication.md

+
+## DESCRIPTION
+Updates an existing azure active directory application.
+To update the credentials associated with this application, please use New-AzureRmADAppCredential cmdlet.


<nit>
.. please use the New-AzureRmADAppCredential...
</nit>

markcowl · 2018-04-23T17:43:20Z

src/ResourceManager/Resources/Commands.Resources/help/Get-AzureRmResource.md

-Get-AzureRmResource [-ResourceType <String>] [-ExtensionResourceType <String>] [-ExpandProperties]
- [-IsCollection] [-ODataQuery <String>] [-ResourceGroupName <String>] [-ApiVersion <String>] [-Pre]
- [-DefaultProfile <IAzureContextContainer>] [<CommonParameters>]
+PS C:\> Get-AzureRmResource -Name testVM | ft


This output is | fl

markcowl · 2018-04-23T17:50:32Z

...rceManager/Resources/Commands.Resources/ActiveDirectory/NewAzureADServicePrincipalCommand.cs

+                {
+                    try
+                    {
+                        Thread.Sleep(5000);


We should use our own TestMockSupport.Delay method so this doesn't take forever in playback

markcowl

One yaml help issue, otherwise, lgtm

markcowl · 2018-04-24T06:12:34Z

src/ResourceManager/Resources/Commands.Resources/help/Remove-AzureRmADGroupMember.md

+```
+
+### -MemberUserPrincipalName
+The UPN of the member(s) to remove.```yaml


RandalliLama · 2018-04-24T16:17:35Z

I'm taking a look. Will update in an hour.

darshanhs90 · 2018-04-24T20:05:07Z

src/Common/Commands.Common.Graph.RBAC/ActiveDirectory/ActiveDirectoryClientExtensions.cs

@@ -59,7 +59,8 @@ public static PSADObject ToPSADObject(this AADObject obj)
                    DisplayName = obj.DisplayName,
                    Type = obj.ObjectType,
                    Id = new Guid(obj.ObjectId),
-                    SecurityEnabled = obj.SecurityEnabled
+                    SecurityEnabled = obj.SecurityEnabled,
+                    MailNickname = obj.Mail


MailNickname = obj.Mail [](start = 20, length = 23)

for a group the mail will be always null,do we need this?
https://msdn.microsoft.com/en-us/library/azure/ad/graph/api/groups-operations

darshanhs90 · 2018-04-24T20:08:26Z

...Manager/Resources/Commands.Resources/ActiveDirectory/UpdateAzureADServicePrincipalCommand.cs

+    /// Updates an exisitng service principal.
+    /// </summary>
+    [Cmdlet(VerbsData.Update, "AzureRmADServicePrincipal", DefaultParameterSetName = ParameterSet.SpObjectIdWithDisplayName, SupportsShouldProcess = true), OutputType(typeof(PSADServicePrincipal))]
+    [Alias("Set-AzureRmADServicePrincipal")]


Set-AzureRmADServicePrincipa [](start = 12, length = 28)

why the change from Set to update?
isnt it confusing/inconsistent wrt other commandlets?
for some we have set and for others we have update

…ayback

…o resources-improvements # Conflicts: # tools/StaticAnalysis/Exceptions/BreakingChangeIssues.csv # tools/StaticAnalysis/Exceptions/SignatureIssues.csv

…ve logic for Get-AzureRmResource into ProcessRecord

markcowl · 2018-04-26T22:44:06Z

...Resources/Commands.ResourceManager/Cmdlets/Implementation/Resource/GetAzureResourceCmdlet.cs

+            return result;
+        }
+
+        private IEnumerable<PSResource> FilterResourceByWildcard(IEnumerable<PSResource> result)


For the future, we should implement this once as a generic, and pass in a Function that gets the key we want to search

markcowl · 2018-04-27T04:07:16Z

nightly build (Cloudshell prereq) here: https://azuresdkci.westus2.cloudapp.azure.com/view/PowerShell/job/powershell-nightly/122/

cormacpayne · 2018-04-27T15:15:40Z

On-demand: https://azuresdkci.westus2.cloudapp.azure.com/job/powershell-demand/781/

…ng called

cormacpayne · 2018-04-30T15:23:18Z

On-demand: https://azuresdkci.westus2.cloudapp.azure.com/job/powershell-demand/791/ (and 792)
Sign job: https://azuresdkci.westus2.cloudapp.azure.com/job/ps-sign/511/

RandalliLama · 2018-04-24T17:59:49Z

src/ResourceManager/KeyVault/Commands.KeyVault/Models/KeyVaultManagementCmdletBase.cs

@@ -265,7 +265,8 @@ private string GetObjectIdBySpn(string spn)
            if (!string.IsNullOrWhiteSpace(spn))
            {
 #if NETSTANDARD
-                var servicePrincipal = ActiveDirectoryClient.FilterServicePrincipals(new ADObjectFilterOptions() { SPN = spn }).SingleOrDefault();
+                var odataQuery = new Rest.Azure.OData.ODataQuery<Graph.RBAC.Version1_6.Models.ServicePrincipal>(s => s.ServicePrincipalNames.Contains(spn));


Is this filter using a case insensitive compare like the one below on line 273? If not, is there a reason that it would not be using the same?

@RandalliLama the filter is case insensitive

cormacpayne · 2018-04-30T20:06:55Z

CloudShell test: https://azuresdkci.westus2.cloudapp.azure.com/job/ps-cloudshell-test/134/

cormacpayne added 7 commits April 20, 2018 09:23

AD code changes

9fd8920

AD markdown changes

ab72618

ARM code changes

42620ad

ARM markdown changes

9aa487e

Misc minor markdown changes

915e926

Common code changes

b6e25ef

Other minor changes

744e60e

azuresdkci added the needs-review label Apr 20, 2018

cormacpayne assigned markcowl and darshanhs90 Apr 20, 2018

Fix compilation errors in build, suppress some breaking change except…

c8957df

…ions

markcowl requested changes Apr 23, 2018

View reviewed changes

markcowl assigned maddieclayton Apr 23, 2018

markcowl assigned cormacpayne Apr 23, 2018

markcowl requested changes Apr 23, 2018

View reviewed changes

dragav requested a review from RandalliLama April 23, 2018 21:14

Resolve review comments

0bb3cfe

cormacpayne unassigned cormacpayne, darshanhs90 and maddieclayton Apr 24, 2018

markcowl approved these changes Apr 24, 2018

View reviewed changes

markcowl previously approved these changes Apr 24, 2018

View reviewed changes

markcowl removed their assignment Apr 24, 2018

markcowl added the needs-revision label Apr 24, 2018

markcowl assigned cormacpayne Apr 24, 2018

Update Remove-AzureRmADGroupMember.md

be18063

cormacpayne dismissed markcowl’s stale review via be18063 April 24, 2018 14:41

darshanhs90 reviewed Apr 24, 2018

View reviewed changes

cormacpayne added 5 commits April 25, 2018 13:55

Re-record failing Resources tests, minor fixes across cmdlets

3a55299

Fix error in Resources test .csproj

fe33929

Re-record final failing tests, temporarily skip three AD tests

5538d41

Skip tests that are successfully re-recorded, but still failing in pl…

661bdf0

…ayback

Merge branch 'release-6.0.0' of github.com:Azure/azure-powershell int…

a8671f0

…o resources-improvements # Conflicts: # tools/StaticAnalysis/Exceptions/BreakingChangeIssues.csv # tools/StaticAnalysis/Exceptions/SignatureIssues.csv

maddieclayton changed the base branch from preview to release-6.0.0 April 26, 2018 21:37

Update Scope and ResourceType completers to get contained strings, mo…

e45894f

…ve logic for Get-AzureRmResource into ProcessRecord

markcowl reviewed Apr 27, 2018

View reviewed changes

Remove deprecated functionality from Get-AzureRmResource that was bei…

b665d29

…ng called

RandalliLama reviewed Apr 30, 2018

View reviewed changes

RandalliLama previously approved these changes Apr 30, 2018

View reviewed changes

Re-record failing / flaky AD test

f8534ab

cormacpayne dismissed RandalliLama’s stale review via f8534ab April 30, 2018 20:11

Update RaPropertiesValidation test with new subscription id

f743f2f

markcowl approved these changes Apr 30, 2018

View reviewed changes

markcowl merged commit e590c75 into Azure:release-6.0.0 Apr 30, 2018

cormacpayne deleted the resources-improvements branch May 2, 2018 23:54

Resources improvements #6008

Resources improvements #6008

Uh oh!

Conversation

cormacpayne commented Apr 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

markcowl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markcowl commented Apr 23, 2018

Uh oh!

markcowl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cormacpayne Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markcowl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RandalliLama commented Apr 24, 2018

Uh oh!

cormacpayne commented Apr 20, 2018 •

edited

Loading

cormacpayne Apr 23, 2018 •

edited

Loading

cormacpayne commented Apr 30, 2018 •

edited

Loading