Estebanreyl/admin enabled fixes #341

estebanreyl · 2018-07-31T00:26:12Z

-Fixes #336, fixes #359

Eliminated bugs related to failing to fetch a single container registry for any particular reason.
Eliminated the need for registries to be admin enabled at all to be listed in Azure Container Registries explorer.
Provided a fix for launching to app services after the aforementioned change and an overall fix due to a very obscure bug that prevented launches when an available resource group had a location outside of the locations available for a subscription.
As a side note rather than a feature, speed for container registries has further increased as a result of admin enabled elimination.

…es deployment -Additionally included code for future updates of Azure app services when non admin enabled registries are allowed.

…ch available resourcegroup.

northtyphoon · 2018-07-31T20:11:55Z

explorer/models/azureRegistryNodes.ts

-                    node.userName = element.userName;
-                    node.created = moment(new Date(JSON.parse(manifest.history[0].v1Compatibility).created)).fromNow();
-                    imageNodes.push(node);
+                    let data: any;


looks data is only used in try-catch block?

northtyphoon · 2018-07-31T20:17:51Z

explorer/deploy/webAppCreator.ts

@@ -172,9 +177,16 @@ class ResourceGroupStep extends WebAppCreatorStepBase {
            resourceGroups = results[0];
            locations = results[1];
            resourceGroups.forEach(rg => {
+                let location: string;
+                try {
+                    location = `(${locations.find(l => l.name.toLowerCase() === rg.location.toLowerCase()).displayName})`;


` [](start = 31, length = 1)

is backtick needed here?

Sorry, that was a leftover from before

northtyphoon · 2018-07-31T20:25:30Z

explorer/deploy/webAppCreator.ts

@@ -172,9 +177,16 @@ class ResourceGroupStep extends WebAppCreatorStepBase {
            resourceGroups = results[0];
            locations = results[1];
            resourceGroups.forEach(rg => {
+                let location: string;
+                try {
+                    location = `(${locations.find(l => l.name.toLowerCase() === rg.location.toLowerCase()).displayName})`;


location [](start = 20, length = 8)

You might want to change the name to locationDisplayName or locationDescription.

northtyphoon · 2018-07-31T20:27:15Z

explorer/deploy/webAppCreator.ts

+            this._serverPassword = context.password;
+            this._serverUserName = context.userName;
+        }
+        if (context instanceof AzureImageNode) {


if [](start = 8, length = 2)

else if to avoid additional check

you also might to log if context is neither DockerHubImageNode or AzureImageNode

In reply to: 206671989 [](ancestors = 206671989)

northtyphoon · 2018-07-31T20:31:26Z

explorer/deploy/webAppCreator.ts

@@ -447,6 +468,12 @@ class WebsiteStep extends WebAppCreatorStepBase {
                await vscode.window.showWarningMessage(nameAvailability.message);
            }
        }
+        try {
+            await this.loginCredentials();


loginCredentials [](start = 23, length = 16)

Can you rename to something like acquireRegistryLoginCredentials? It's not clear from the name what credentials are about since it's in WebsiteStep class.

That seems reasonable to me

northtyphoon · 2018-07-31T20:35:51Z

explorer/deploy/webAppCreator.ts

+            }
+        }
+        await vscode.window.showErrorMessage('Azure App service currently only supports running images from Azure Container Registries with admin enabled');
+        throw new Error(('Admin not enabled'));


Admin [](start = 26, length = 5)

Azure Container Registry Admin is not enabled.

northtyphoon · 2018-07-31T20:37:24Z

explorer/deploy/webAppCreator.ts

+        // Un-Comment when Admin enabled is not required
+        // let convertedCredentials: { password: string, username: string } = await this.getSessionCredentials();
+        // this._serverUserName = convertedCredentials.password;
+        // this._serverPassword = convertedCredentials.username;


Is the support of service principal ready?

It can be done on our side and it will probably be added on a per feature thing as we add functionality such as pull and push but app services itself does not seem to support them at the moment.

northtyphoon

🕐

Removed methods that would be used if App services started supporting non admin enabled registries. These were removed to avoid confusions.

estebanreyl · 2018-07-31T21:27:49Z

@sajayantony

ankurkhemani · 2018-08-01T00:23:42Z

explorer/deploy/webAppCreator.ts

 import WebSiteManagementClient = require('azure-arm-website');
 import * as fs from 'fs';
 import * as path from 'path';
+import * as request from 'request-promise';


unused import, remove.

ankurkhemani · 2018-08-01T00:24:00Z

explorer/deploy/webAppCreator.ts

 import * as vscode from 'vscode';
 import * as WebSiteModels from '../../node_modules/azure-arm-website/lib/models';
 import { reporter } from '../../telemetry/telemetry';
+import { AzureAccount, AzureLoginStatus, AzureSession } from '../../typings/azure-account.api';


unused import, remove.

ankurkhemani · 2018-08-01T00:32:53Z

explorer/deploy/webAppCreator.ts

+                this._serverUserName = creds.username;
+                return;
+            } catch (error) {
+                vscode.window.showErrorMessage(error.message);


In this case await pauses execution until the user clicks to exit the error message window which is not a guarantee, additionally it makes no difference as the error is shown when ready and execution ends nonetheless.

We shouldn't need to both show a message and throw an error - the error should get displayed by the caller, is that not happening?

One issue is that this code was copied from elsewhere and we need to actually update it to use the shared code in https://github.com/Microsoft/vscode-azuretools, so maybe that needs to be done before integrating these changes.

This does not happen indeed, in fact there is no visible information about an error occurring if I propagate it all the way to the code I didn't write, although I could modify it to act in this way. Additionally, while I agree that we should update the original code for the whole deploy to app services, this PR is essentially an important bugfix which by the nature of the bug broke the existing app services functionality and thus required me to solve a small number of issues on that side to keep existing functionality post the PR. Would it not be best to integrate this functional code with the added bugfixes before attempting the necessary refactoring that appears to me as less pressing?

Makes sense. Added #358 to track that we want telemetry for such failures.

BTW, with my merge of #350, the caller should now be catching and handling (telemetry and error messages) anything you throw from commands. That should now be the correct way to handle things (let exceptions bubble up), except for cases like there where you need to continue.

sweet, Ill update accordingly

ankurkhemani · 2018-08-01T00:35:49Z

explorer/deploy/webAppCreator.ts

+
+        if (this._registry.adminUserEnabled) {
+            try {
+                let creds = await client.registries.listCredentials(resourceGroup, this._registry.name);


move this to _util file inside a new function listRegistryCredentials ?

While this makes sense to me conceptually I am honestly having trouble with how to go about it, I could create a function say in azureUtils that takes an azure account, a subscription, and a registry to do this and while this seems reasonable, and maybe I am biased because of PR #332 , it doesn't seem to follow the existing pattern of this file and seems more like something to add to my unified credentials that would look odd anywhere else. It seems most reasonable to me to add it to the Unified credentials branch and leave this like it is, at some point it may be good to refactor this code around that other branch however but that goes for the overall file,

Sounds good, you can consider refactoring in the other branch. Also, you are right about the overall file in general.

ankurkhemani

LGTM

StephenWeatherford · 2018-08-01T17:35:46Z

explorer/deploy/webAppCreator.ts

+                let locationDisplayName: string;
+                try {
+                    locationDisplayName = locations.find(l => l.name.toLowerCase() === rg.location.toLowerCase()).displayName;
+                } catch (error) {


What error is occurring? Null ref? I'd rather avoid exceptions than catch and recover.

Exactly, I could also store the result in a variable and check if it is available before assigning the value, say :
const location : Location = locations.find(l => l.name.toLowerCase() === rg.location.toLowerCase());

if(location){
locationDisplayName = location.displayName;
}else{
...
}

Alternatively I could try changing the location acquisition by subscription strategy and do so by overall locations which would prevent the error in the grand majority of the very rare cases although I suspect this would have problems in cases when there is a private azure location. Also, is there a specific reason as to prefer avoiding rather than catch and recover?

Yes, that's preferable, thanks. Exceptions should be for things that are exceptional. Really you should never see a null ref exception, it's completely avoidable with proper checking. Having the location not match is a fully normal event that we just need to handle.

(Also, having a bunch of exceptions throw all over the place is darn annoying when you're trying to debug.)

Sounds good, I'll try to follow that paradigm.

StephenWeatherford · 2018-08-01T17:44:46Z

explorer/models/registryRootNode.ts

+                            'registries': regs
+                        });
+                    } catch (error) {
+                        vscode.window.showErrorMessage(error);


Let caller show error.

and other places.

Let me know if the caller is not showing the error. There are further changes around command registration and error handling that I intend to make.

Do you mean handle this within asyncpool? I could do that so its a single location for this.

Let's keep it as is for now.

Except change to show parseError(error).message

PrashanthCorp · 2018-08-01T18:08:36Z

explorer/models/azureRegistryNodes.ts

+                        imageNodes.push(node);
+
+                    } catch (error) {
+                        vscode.window.showErrorMessage(error);


I'm trying to understand where you expect the error to be coming from. Is it from the parse, or the request? I don't see lines 209-213 causing an error.

If you're surer of where the error is expected to come from, it might be better to wrap just that line in a catch.

I anticipate it would come from the request , that seems reasonable, I think I just moved it to the try to avoid having an additional check in case it failed but it is more unreadable.

StephenWeatherford · 2018-08-06T21:44:43Z

explorer/deploy/webAppCreator.ts

+            await this.acquireRegistryLoginCredentials();
+        } catch (error) {
+            //Admin was not enabled, cannot proceed
+            throw new Error(('Admin not enabled'));


nit: double parens

StephenWeatherford · 2018-08-06T21:46:12Z

explorer/deploy/webAppCreator.ts

+            await this.acquireRegistryLoginCredentials();
+        } catch (error) {
+            //Admin was not enabled, cannot proceed
+            throw new Error(('Admin not enabled'));


You're making an assumption about why this failed - it could fail for any number of reasons, including network out - this message could mislead users.

You should simply not have the try/catch here - let the exception bubble up (sync up with my PR I mentioned above).

StephenWeatherford · 2018-08-06T21:48:50Z

explorer/deploy/webAppCreator.ts

+                throw new Error(('Azure Container Registry Admin is not enabled'));
+            }
+        }
+        vscode.window.showErrorMessage('Azure App service currently only supports running images from Azure Container Registries with admin enabled');


Again, you can't make that assumption. Does the error that you get back in this scenario have a particular code that you can check for in order to throw a more specific error in this case?

I could check for whether the list failed because of admin not enabled specifically and then throw accordingly. As for the Azure App service does not support... etc I have to make the assumption as the only way to identify this error is to go into the azure portal and note that your web app fails due to admin not being enabled, no error is actually given to the user from the API as long as credentials are valid.

I'm confused - you're throwing this error specifically when you get an exception. Can you not check for the error code?

StephenWeatherford · 2018-08-06T21:49:16Z

explorer/deploy/webAppCreator.ts

+                throw new Error(('Azure Container Registry Admin is not enabled'));
+            }
+        }
+        vscode.window.showErrorMessage('Azure App service currently only supports running images from Azure Container Registries with admin enabled');


Just throw (use the longer message), don't show a message.

…es deployment -Additionally included code for future updates of Azure app services when non admin enabled registries are allowed.

…ch available resourcegroup.

Removed methods that would be used if App services started supporting non admin enabled registries. These were removed to avoid confusions.

…AzureCR/vscode-docker into estebanreyl/admin-enabled-fixes

estebanreyl · 2018-08-08T03:10:38Z

Ive updated the error handling, nonetheless, bubbling up all the way to the item in command registration does not seem to yield either the telemetry logging (based on console output I've seen) or an error for the user. Note I have rebased my branch with master. I am manually outputting the error to the user however at the highest level possible, let me know if this is better.

StephenWeatherford · 2018-08-09T00:42:04Z

@estebanreyl Okay, I see the problem. I've pushed a fix into your branch.

StephenWeatherford · 2018-08-09T01:03:18Z

explorer/models/azureRegistryNodes.ts

+                            }
+                        });
+                    } catch (error) {
+                        vscode.window.showErrorMessage(error);


vscode.window.showErrorMessage(parseError(error).message);

StephenWeatherford · 2018-08-09T01:11:13Z

Okay, just a couple minor changes left. The wizard code has much improved since it was copied into this repo. Thx.

…-enabled-fixes

estebanreyl · 2018-08-10T01:13:47Z

Okay, I think its ready, just fixed the merge conflicts as well.

StephenWeatherford · 2018-08-10T21:24:11Z

What happened to my error-handling fix (setting output channel) from b28431b?

estebanreyl · 2018-08-10T21:28:52Z

Oh I was confused about that, Ill add it now. Think I removed it on the merge by mistake.

estebanreyl · 2018-08-10T21:40:59Z

@StephenWeatherford I added the set output channel again.

Esteban Rey added 3 commits July 30, 2018 09:51

Fixes crashing of registry opening

ad59f03

Eliminated ACR Admin enabled requirement and fixed this for appservic…

8b5e95c

…es deployment -Additionally included code for future updates of Azure app services when non admin enabled registries are allowed.

Fixed obscure bug that prevented proper showing of the location of ea…

7a75c96

…ch available resourcegroup.

estebanreyl mentioned this pull request Jul 31, 2018

Mention admin accounts for ACR in readme. #340

Closed

northtyphoon reviewed Jul 31, 2018

View reviewed changes

northtyphoon suggested changes Jul 31, 2018

View reviewed changes

Esteban Rey added 2 commits July 31, 2018 13:56

Removed confusing methods for potential future use.

949437b

Removed methods that would be used if App services started supporting non admin enabled registries. These were removed to avoid confusions.

Small naming and style improvements

ce3e2e1

northtyphoon approved these changes Jul 31, 2018

View reviewed changes

ankurkhemani suggested changes Aug 1, 2018

View reviewed changes

Removed unused imports

e58cfa7

ankurkhemani approved these changes Aug 1, 2018

View reviewed changes

StephenWeatherford reviewed Aug 1, 2018

View reviewed changes

PrashanthCorp reviewed Aug 1, 2018

View reviewed changes

Small style updates

803a8ef

StephenWeatherford reviewed Aug 6, 2018

View reviewed changes

StephenWeatherford mentioned this pull request Aug 6, 2018

Show non-admin-enabled Azure container registries in tree #359

Closed

Esteban Rey added 2 commits August 7, 2018 16:13

Fixes crashing of registry opening

f9b4bd1

Eliminated ACR Admin enabled requirement and fixed this for appservic…

dc47180

…es deployment -Additionally included code for future updates of Azure app services when non admin enabled registries are allowed.

Esteban Rey added 7 commits August 7, 2018 16:13

Fixed obscure bug that prevented proper showing of the location of ea…

06226e1

…ch available resourcegroup.

Removed confusing methods for potential future use.

3ebe79a

Removed methods that would be used if App services started supporting non admin enabled registries. These were removed to avoid confusions.

Small naming and style improvements

39cc9cf

Removed unused imports

790cac6

Small style updates

0c2377d

Merge branch 'estebanreyl/admin-enabled-fixes' of https://github.com/…

2c67a8c

…AzureCR/vscode-docker into estebanreyl/admin-enabled-fixes

Updated Error checking

0596c30

Stephen Weatherford added 2 commits August 8, 2018 17:56

Fix error handling

b28431b

user canceled

253083d

StephenWeatherford reviewed Aug 9, 2018

View reviewed changes

Esteban Rey added 2 commits August 9, 2018 17:20

Adds parseError

ff38b2c

Merge remote-tracking branch 'upstream/master' into estebanreyl/admin…

07a42cb

…-enabled-fixes

added ext.outputchannel back

ba24c82

StephenWeatherford approved these changes Aug 10, 2018

View reviewed changes

StephenWeatherford merged commit 1d98281 into microsoft:master Aug 11, 2018

microsoft locked and limited conversation to collaborators Oct 27, 2021

Estebanreyl/admin enabled fixes #341

Estebanreyl/admin enabled fixes #341

Conversation

estebanreyl commented Jul 31, 2018 • edited by StephenWeatherford 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

northtyphoon left a comment

Choose a reason for hiding this comment

estebanreyl commented Jul 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebanreyl Aug 1, 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

ankurkhemani left a 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

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

estebanreyl Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebanreyl commented Aug 8, 2018

StephenWeatherford commented Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

StephenWeatherford commented Aug 9, 2018

estebanreyl commented Aug 10, 2018

StephenWeatherford commented Aug 10, 2018

estebanreyl commented Aug 10, 2018 • edited Loading

estebanreyl commented Aug 10, 2018

estebanreyl commented Jul 31, 2018 •

edited by StephenWeatherford

Loading

estebanreyl Aug 1, 2018 •

edited

Loading

estebanreyl Aug 6, 2018 •

edited

Loading

StephenWeatherford commented Aug 9, 2018 •

edited

Loading

estebanreyl commented Aug 10, 2018 •

edited

Loading