[ACR] Decouple reliance on Commands.Resources.Rest #5805
Conversation
| @@ -19,6 +19,7 @@ | |||
| --> | |||
| ## Current Release | |||
| * Fix issue with Default Resource Group in CloudShell | |||
There was a problem hiding this comment.
@djyou it looks like you need to pull in the latest changes from the preview branch since this line should be under the Version 1.0.3 header
| @@ -12,7 +12,6 @@ | |||
| // limitations under the License. | |||
| // ---------------------------------------------------------------------------------- | |||
|
|
|||
| using Microsoft.Azure.Commands.ResourceManager.Common.ArgumentCompleters; | |||
There was a problem hiding this comment.
@djyou I think we should keep this using and add the ResourceGroupCompleter attribute to the ResourceGroupName parameter below.
@maddieclayton any idea why the using was added but the completer wasn't?
There was a problem hiding this comment.
I don't think we have implementation for resource group completer. As this is a cleanup PR I would prefer not making functional changes such as adding resource group completer. I meant to remove all unused using here and in other places.
There was a problem hiding this comment.
@cormacpayne Looks like it was lost in a bad merge. @djyou This resourcegroupcompleter is available across all cmdlets, and lets users easily tab through resource groups in their current subscription. It was added here: 8f17953#diff-baa9c46b222beec8be060e1dd8694915, but somehow lost in the following PR in a merge. Can you please add it back to GetAzureContainerRegistryCredential, NewAzureContainerRegistry, and UpdateAzureContainerRegistryCredential?
| using Microsoft.Azure.Management.ContainerRegistry.Models; | ||
| using Microsoft.Azure.Commands.ResourceManager.Common.ArgumentCompleters; |
There was a problem hiding this comment.
@djyou same comment here about the ResourceGroupCompleter
| { | ||
| _client = AzureSession.Instance.ClientFactory.CreateArmClient<ResourceManagementClient>(context, AzureEnvironment.Endpoint.ResourceManager); |
There was a problem hiding this comment.
@djyou we should remove all dependencies external to your service where we can, which would include this dependency on the Microsoft.Azure.Management.ResourceManager package. Is there any reason that you are using this package instead of the common ARM code we have in Commands.ResourceManager.Common?
There was a problem hiding this comment.
Are you suggesting we should also remove references such as https://github.com/Azure/azure-powershell/blob/preview/src/ResourceManager/ContainerRegistry/Commands.ContainerRegistry/Commands.ContainerRegistry.csproj#L54?
There was a problem hiding this comment.
@djyou correct, your project shouldn't be dependent on anything but SDKs that your service manages and common code projects.
|
@cormacpayne Comments should have been addressed. @maddieclayton Added |
Fixes #5775
Description
Checklist
CONTRIBUTING.mdplatyPSmodule