Azurecr/registry management #383

estebanreyl · 2018-08-16T22:23:58Z

Adds ACR management capabilities to the extension, including:

Create Registry
Delete Registry
Delete Repository closes Allow removing a repository from ACR #172
Delete Image

Internally provides additional functionality for further development including:
Quick picks for azure related items

Subscription
ACR Registry
ACR Repository
ACR Image
Resource Group
ACR SKU
Locations

Additional functionality for ACR

Refresh Token acquisition for docker login
Access Token acquisition for v2 api
Acquire images, repositories
Acquire subscription from registry

Other changes:

Unified client now lists only non classic acr registries

Update dev to match Microsoft/vscode-docker/master

* Added Azure Credentials Manager Singleton * Added getResourceManagementClient

Added try catch loop and awaited for resource group list again to check for duplicates with ResourceManagementClient

* Added Azure Credentials Manager Singleton * Small Style Fixes * Further Style fixes, added getResourceManagementClient * Lazy Initialization Patches

-Changed order of questions asked to user for better UX (location of new res group & location of new registry) -Placeholder of location is display name view

Master update

Create Registry

* first fully functional version of delete through input bar AND right click * refactored code to make it prettier! * comments * comments, added subscription function * fixed to style guide * style fixes, refactoring * delete image after reviews put my functions from azureCredentialsManager into two new files: utils/azure/acrTools.ts, and commands/utils/quick-pick-azure.ts Edited code based on Esteban's and Bin's reviews * One last little change to delete image * moved repository, azureimage, and getsubscriptions to the correct places within deleteImage * changes from PR reviews on delete image * fixed authentication issue, got rid of azureAccount property for repository and image **on constructor for repository, azurecredentialsmanager was being recreated and thus couldn't find the azureAccount. For this reason, I got rid of the azureAccount property of the classes Repository and AzureImage. This bug may lead to future problems (Esteban and I couldn't see why it was happening) * minor fixes deleteImage * delete a parentheses

Master merge maintain fork updated

* Merge fixes to acquire latest telemetry items * Updated to master AzureUtilityManager

* tslint updates, transfered from old branch * updated icon * Addressed PR comments- mostly styling/code efficiency * Changed url to aka.ms link * changed Error window to Info message for no build tasks in your registry * Changed default sku and unified parsing resource group into a new method, getResourceGroup in acrTools.ts * Changed build task icon

* deleteRepo moved over to branch off dev * Got rid of unnecessary code, fully functioning! * deleteRepo moved over to branch off dev * Got rid of unnecessary code, fully functioning! * spacing * final commit * Cleaned code * Added Telemetry

Delete azure registry functionality added Delete azure registry moved to branch off dev Reorganized stye

Master dev merge

StephenWeatherford · 2018-08-22T19:53:47Z

package.json

@@ -491,6 +511,21 @@
        "description": "Restarts a composition of containers",
        "category": "Docker"
      },
+      {
+        "command": "vscode-docker.create-ACR-Registry",
+        "title": "Create Registry",


"title": "Create Azure Registry..."

The "Azure" part is important because from the command palette the context would otherwise be unclear. The "..." lets the user know we will be asking for more information.

One thing my team suggested was should we maybe instead change the category to ACR ? And do the same for the rest?

I'd say it should probably match the registry node name, which is currently 'Azure'. You thinking that should change?

Yes, our pm suggested that we could just leave it so the commands would show up as ACR : Create Registry, ACR : Delete Repository, etc …

@fiveisprime Matt, what are your thoughts on having the ACR commands use a different category than the rest? Personally I'd rather they all start with "Docker: ".

(Esteban - I misspoke earlier about the category matching the node - I was referring to the command title, not the category).

I.e., I'd suggest:
Docker: Create ACR Registry
Docker: Delete ACR Repository
...

I would rather than start with "Docker:" as well; I like the suggestion of "Docker: Create ACR Registry" and so on. Is that clear enough?

Should we still abbreviate in this case? For example, would it make more sense to say "Docker: Create Azure Container Registry"?

I think either would be fine, I'll try to message my team's PM to see what his opinion is.

StephenWeatherford · 2018-08-22T19:58:46Z

commands/utils/quick-pick-azure.ts

+        resourceGroupNames.push(resGroupName.name);
+    }
+
+    let resourceGroupName = await vscode.window.showQuickPick(resourceGroupNames, {


ext.ui.showQuickPick().

With your current code, the user can press cancel on this step, but the code keeps going and eventually ends up with "Error: Cannot read property 'name' of undefined"

StephenWeatherford · 2018-08-22T20:05:57Z

commands/utils/quick-pick-azure.ts

+    let resourceGroupNames: string[] = [];
+
+    if (canCreateNew) { resourceGroupNames.push('+ Create new resource group'); }
+    for (let resGroupName of resourceGroups) {


resGroupName -> resGroup (it's a ResourceGroup, not a string)

StephenWeatherford · 2018-08-22T20:13:48Z

commands/utils/quick-pick-azure.ts

+    } else {
+        resourceGroup = resourceGroups.find(resGroup => { return resGroup.name === resourceGroupName; });
+    }
+    return resourceGroup;


This method has some possible bug points that result from that fact that:

You're treating the first item as special and indexing resourceGroupNames[0] directly

You're assuming you will find the resourceGroup by name

These are fine as currently written, but they're fragile. E.g., what would you theoretically do you do if the find returns undefined? You can avoid these issues completely by rewriting this way:

export async function quickPickResourceGroup(canCreateNew?: boolean, subscription?: Subscription): Promise<ResourceGroup> { let resourceGroups = await AzureUtilityManager.getInstance().getResourceGroups(subscription); let resourceGroupItems: QuickPickItemWithData<ResourceGroup | undefined>[] = []; let createNewItem: QuickPickItemWithData<ResourceGroup | undefined> = { label: '+ Create new resource group', data: undefined }; if (canCreateNew) { resourceGroupItems.push(createNewItem); } for (let resGroup of resourceGroups) { resourceGroupItems.push({ label: resGroup.name, data: resGroup }); } let selectedItem = await vscode.window.showQuickPick(resourceGroupItems, { 'canPickMany': false, 'placeHolder': 'Choose a resource group to use' }); let resourceGroup: ResourceGroup; if (selectedItem === createNewItem) { if (!subscription) { subscription = await quickPickSubscription(); } const loc = await quickPickLocation(subscription); resourceGroup = await createNewResourceGroup(loc, subscription); } else { resourceGroup = selectedItem.data; } return resourceGroup; }

Thanks! that looks a lot better and is more stable, I also implemented it for the optional can create part of quickPickACRRegistry. As a note I used IAzureQuickPickItem instead of QuickPickWithData as the IAzure one keeps track of recently used items.

StephenWeatherford · 2018-08-22T20:17:46Z

utils/Azure/acrTools.ts

+        });
+    });
+}
+/** Obtains refresh tokens for Azure Container Registry. */


nit: It's easier to read if you separate each function with a blank line

StephenWeatherford · 2018-08-22T20:18:58Z

utils/Azure/acrTools.ts

+
+//Credential management
+/** Obtains registry username and password compatible with docker login */
+export async function loginCredentials(registry: Registry): Promise<{ password: string, username: string }> {


This appears to be unused.

It is as of this PR but I included it given that all the other token management items are included in this one. It is used in an upcoming feature we will PR after this one to enable pull.

StephenWeatherford · 2018-08-22T20:24:16Z

utils/Azure/acrTools.ts

@@ -0,0 +1,163 @@
+/*---------------------------------------------------------------------------------------------


This appears to be largely a duplicate of code in azureRegistryNodes.ts. Duplicate code is bad - what if a bug is fixed in one copy but left unfixed in another? If you want to pull out the code from azureRegistryNodes.ts into shared functions and call it from multiple places, that's fine, as long the actual logic exists only in one place.

Seems reasonable, Ill get to that too.

StephenWeatherford · 2018-08-22T20:34:50Z

commands/utils/quick-pick-azure.ts

+
+export async function quickPickACRRegistry(canCreateNew: boolean = false, prompt?: string): Promise<Registry> {
+    const placeHolder = prompt ? prompt : 'Choose registry to use';
+    let registries = await AzureUtilityManager.getInstance().getRegistries();


You're not passing in a sort function, there's the list is not sorted. Really this is a problem with AzureUtilityManager.getRegistries - it should either:

require a sort function (not optional)

have a default sort if none is provided

or don't ask for a sort function, just always do a sort by name

The current implementation makes bugs like these likely.

I think option 2 is best. It provides the functionality without limiting options that will avoid double sorting if an alternate sort is necessary. Ill use a default alphabetical sort.

StephenWeatherford · 2018-08-22T20:45:00Z

commands/azureCommands/delete-image.ts

+        vscode.window.showInformationMessage(`Successfully deleted image ${tag}`);
+        if (context) {
+            dockerExplorerProvider.refreshNode(context.parent);
+        }


else refreshRegistries()

StephenWeatherford · 2018-08-22T20:45:09Z

commands/azureCommands/delete-repository.ts

+        vscode.window.showInformationMessage(`Successfully deleted repository ${Repository.name}`);
+        if (context) {
+            dockerExplorerProvider.refreshNode(context.parent);
+        }


else refreshRegistries()

StephenWeatherford

See additional comments

StephenWeatherford · 2018-08-22T23:13:23Z

commands/utils/quick-pick-azure.ts

+export async function confirmUserIntent(yesOrNoPrompt: string): Promise<boolean> {
+    let opt: vscode.InputBoxOptions = {
+        ignoreFocusOut: true,
+        placeHolder: 'No',


placeHolder: 'Yes'

(Value is the default value - they'll see that at first - so 'No' is correct for that. placeHolder is a watermark which they'll see if they delete the 'No' default value - it guides them or gives them examples of what to enter.)

Oh that makes sense

…ixes

…cally

estebanreyl · 2018-08-23T02:33:01Z

I've updated to match the review items, let me know if there is anything else that looks like it could use some improvement. The only thing that may need a bit of further discussion is if we should change the naming for the acr features to show up as ACR : Create Registry, ACR : Delete Repository, etc …changing the category. For now I just normalized create Registry to Create Azure Registry.

StephenWeatherford · 2018-08-24T22:41:26Z

commands/utils/quick-pick-azure.ts

+    let createNewItem: IAzureQuickPickItem<Registry | undefined> = { label: '+ Create new registry', data: undefined };
+    if (canCreateNew) { quickPickRegList.unshift(createNewItem); }
+
+    let desiredReg: IAzureQuickPickItem<Registry | undefined> = await ext.ui.showQuickPick(quickPickRegList, {


ext.ui.showQuickPick will never return undefined (it will throw UserCancelledError instead)

That's for the type of data within IAzureQuickPickItem however. That's only so I can potentially create a new registry if desired, it should have not have an effect on error checking neither is it used towards that effect.

StephenWeatherford · 2018-08-24T22:43:51Z

commands/utils/quick-pick-azure.ts

@@ -128,36 +130,30 @@ export async function quickPickResourceGroup(canCreateNew?: boolean, subscriptio
 export async function confirmUserIntent(yesOrNoPrompt: string): Promise<boolean> {
    let opt: vscode.InputBoxOptions = {
        ignoreFocusOut: true,
-        placeHolder: 'No',
+        placeHolder: 'Yes',


After playing with it, I'm suggesting a change:
placeHolder: 'Enter "Yes"',

Without that, it looks too much like a default value.

StephenWeatherford · 2018-08-24T23:08:02Z

explorer/models/registryRootNode.ts


            const subPool = new AsyncPool(MAX_CONCURRENT_SUBSCRIPTON_REQUESTS);
            let subsAndRegistries: { 'subscription': SubscriptionModels.Subscription, 'registries': ContainerModels.RegistryListResult }[] = [];
            //Acquire each subscription's data simultaneously
-            // tslint:disable-next-line:prefer-for-of // Grandfathered in
-            for (let i = 0; i < subs.length; i++) {
+            for (let sub of subscriptions) {


StephenWeatherford · 2018-08-24T23:13:52Z

commands/azureCommands/delete-image.ts

@@ -33,8 +35,8 @@ export async function deleteAzureImage(context?: AzureImageNode): Promise<void>
        tag = wholeName[1];
    }

-    const shouldDelete = await quickPicks.confirmUserIntent(`Are you sure you want to delete ${repoName}:${tag}? Enter yes to continue: `);
-    if (shouldDelete) {
+    const shouldDelete = await ext.ui.showWarningMessage(`Are you sure you want to delete ${repoName}:${tag}? Enter yes to continue: `, { modal: true }, DialogResponses.deleteResponse, DialogResponses.cancel);


Remove "Enter yes to continue:"

I see you fixed that later...

StephenWeatherford · 2018-08-24T23:16:46Z

commands/azureCommands/delete-registry.ts

+    } else {
+        registry = await quickPickACRRegistry(false, 'Select the registry you want to delete');
+    }
+    const shouldDelete = await confirmUserIntent(`Are you sure you want to delete ${registry.name} and its associated images? Enter yes to continue: `);


nit: You should let confirmUserIntent() append the 'Enter Yes to continue' portion to the prompt, since it's the one that's expecting that answer from the user anyway.

StephenWeatherford · 2018-08-24T23:22:01Z

dockerExtension.ts

@@ -160,6 +165,10 @@ export async function activate(ctx: vscode.ExtensionContext): Promise<void> {
    ctx.subscriptions.push(vscode.debug.registerDebugConfigurationProvider('docker', new DockerDebugConfigProvider()));

    if (azureAccount) {


Because these commands are specified in the package.json, they will show up in the command palette. If you don't register them and the user tries to use them, you'll get an error from vscode saying it couldn't find the command. You could either:

set it up so they don't show up in the command palette (minor pain)

have them show a message to the user that they require the Azure Account extension to be installed

I'd prefer #2 for better discoverability.

I'll update accordingly then.

I added a wrapper for registerCommand called registerAzureCommand that handles the case that the azure account extension is not installed by letting the user know in the same manner as the deploy to appservices prompt does before. I suspect we may want to gather telemetry on this?

StephenWeatherford

Just a few more minor changes, otherwise looking good!

Esteban Rey and others added 30 commits July 25, 2018 10:21

Merge pull request #21 from AzureCR/master

05b3138

Update dev to match Microsoft/vscode-docker/master

Added Azure Credentials Manager Singleton (#18)

a7be0b3

* Added Azure Credentials Manager Singleton * Added getResourceManagementClient

Sorted Existing Create Registry ready for code review

b2e5740

Added acquiring telemetry data for create registry

ef1ccfa

broke up createnewresourcegroup method and fixed client use

1fca0da

Added try catch loop and awaited for resource group list again to check for duplicates with ResourceManagementClient

Jackson esteban/unified client nit Fix (#24)

60f68d3

* Added Azure Credentials Manager Singleton * Small Style Fixes * Further Style fixes, added getResourceManagementClient * Lazy Initialization Patches

Enabled location selection

26f00e2

Location request fixes

fe0fe2f

-Changed order of questions asked to user for better UX (location of new res group & location of new registry) -Placeholder of location is display name view

Refactor while loop for new res group

5c8d364

Added SKU selection

a61c728

Quick fix- initializing array syntax

9b51397

Added specific error messages and comments

048eb67

Merge pull request #30 from AzureCR/master

bc905d8

Master update

Merge pull request #22 from AzureCR/Esteban-Julia-Rutu/CreateRegistry

37d8a27

Create Registry

Merge branch 'dev' into master

e2cf013

Merge pull request #42 from AzureCR/master

64c8282

Master merge maintain fork updated

Estebanreyl/dev merge fixes (#43)

05df67c

* Merge fixes to acquire latest telemetry items * Updated to master AzureUtilityManager

Julia/delete repository final (#49)

0a3d6e1

* deleteRepo moved over to branch off dev * Got rid of unnecessary code, fully functioning! * deleteRepo moved over to branch off dev * Got rid of unnecessary code, fully functioning! * spacing * final commit * Cleaned code * Added Telemetry

Julia/delete registry final (#47)

19f8b72

Delete azure registry functionality added Delete azure registry moved to branch off dev Reorganized stye

Merge branch 'master2' into dev

06b0c99

Merge pull request #52 from AzureCR/master-dev-merge

d694255

Master dev merge

began updating

4bd802b

Reorganized create registry and delete azure image

3d38215

continued improvements

ca723eb

Began updating login

3964731

being credentials update

41429c5

further updates

5d84452

Finished updating, need to test functionality now

da96bf0

StephenWeatherford reviewed Aug 22, 2018

View reviewed changes

StephenWeatherford requested changes Aug 22, 2018

View reviewed changes

StephenWeatherford reviewed Aug 22, 2018

View reviewed changes

Esteban Rey added 3 commits August 22, 2018 17:28

Updated resource group and registry validity checking and other nit f…

96518e7

…ixes

Updated Azure Utility Manager to by default sort registries alphabeti…

24b4d09

…cally

Updated azureRegistryNodes and registryRootNode to use shared functions

014930a

Esteban Rey and others added 3 commits August 22, 2018 20:03

Corrected resourcegroup name test

dd2482c

added delete button when deleting an image

6c5fb36

Small changes in variables for better prompts and success notifications

105a611

StephenWeatherford reviewed Aug 24, 2018

View reviewed changes

StephenWeatherford requested changes Aug 24, 2018

View reviewed changes

Resolve merge conflicts and update no azureAccount issue

998f639

StephenWeatherford approved these changes Aug 27, 2018

View reviewed changes

StephenWeatherford merged commit bc29acd into microsoft:master Aug 27, 2018

microsoft locked and limited conversation to collaborators Oct 27, 2021

		@@ -0,0 +1,163 @@
		/*---------------------------------------------------------------------------------------------

		@@ -160,6 +165,10 @@ export async function activate(ctx: vscode.ExtensionContext): Promise<void> {
		ctx.subscriptions.push(vscode.debug.registerDebugConfigurationProvider('docker', new DockerDebugConfigProvider()));

		if (azureAccount) {

Azurecr/registry management #383

Azurecr/registry management #383

Conversation

estebanreyl commented Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenWeatherford Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenWeatherford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebanreyl commented Aug 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenWeatherford left a comment

Choose a reason for hiding this comment

estebanreyl commented Aug 16, 2018 •

edited

Loading

StephenWeatherford Aug 22, 2018 •

edited

Loading