Show labels for invalid app services #373

sunmorgus · 2018-03-28T17:18:17Z

Updated code from #372 to still allow the invalid resources to be listed along with the valid ones.

ejizba

I'd prefer if you didn't change WebAppTreeItem. That should only be used if the site is valid.

Instead, can you change InvalidTreeItem to be InvalidWebAppTreeItem and have it at the same level as WebAppTreeItem? I also think the code would be clearer if we shifted from several 'filter' and 'map' calls to just a single 'map' call. Here's some pseudo-code:

const treeItems: IAzureTreeItem[] = [];
await Promise.all(webAppCollection
    .map(async (s: Site) => {
        try {
            const siteClient: SiteClient = new SiteClient(s, node);
            // check isFunctionApp and get appServicePlan
            treeItems.push(new WebAppTreeItem(s, appServicePlan));
        } catch {
            treeItems.push(new InvalidWebAppTreeItem(s.name));
        }
    });
return treeItems;

Hopefully then you also won't have to create the fake Site

sunmorgus · 2018-03-29T12:10:13Z

Here's what I've got so far...

Converted to "InvalidWebAppTreeItem" with no children
Added grayscale webapp icon

Couple of questions/input needed...

Can't decide whether to keep the "(invalid)" tag at the end of the label... I'd say having no children and the gray icon would be enough to indicate that there was some sort of problem, but I do like the extra little bit of info.
1. I did attempt to add a child item using the "InvalidTreeItem", but that won't work without having to build the fake Site object like I did before.
Considering attempting to include one or two context menu items for the invalid WebApps...
1. "Open WebApps Portal" (since I can't do "Open In Portal" without a valid id). Not even sure I could do that though, without having full site info
2. A static string item that says "App service configuration is invalid"

ejizba

Looks much better! Just a few minor comments. I think you can do openInPortal pretty easily - just check if site.id is defined on your invalid web apps (If its not, then I just wouldn't do that feature)

ejizba · 2018-03-29T15:13:46Z

src/explorer/WebAppTreeItem.ts

@@ -126,3 +127,20 @@ export class WebAppTreeItem extends SiteTreeItem {
        return await fs.readFile(templatePath, 'utf8');
    }
 }
+
+export class InvalidWebAppTreeItem implements IAzureTreeItem {


Please move this to a separate file "InvalidWebAppTreeItem.ts"

I included it here, as that's the same way the DeploymentSlotsNATreeItem is defined in the DeploymentSlotsTreeItem.js... there's a precedent for doing it like so... should I still move it to a different file?

Yeah @nturinski did that, not me 😊 I generally prefer one exported class per file, since you never know how much a class will grow as time goes on

That being said - this is kind of a stylistic preference and I wouldn't block a PR just for this

Went ahead and moved it to it's own file (I prefer it that way as well, lol).

ejizba · 2018-03-29T15:15:07Z

src/explorer/WebAppTreeItem.ts

@@ -126,3 +127,20 @@ export class WebAppTreeItem extends SiteTreeItem {
        return await fs.readFile(templatePath, 'utf8');
    }
 }
+
+export class InvalidWebAppTreeItem implements IAzureTreeItem {
+    public static contextValue: string = 'file';


I assume you copied this from the FileTreeItem haha? Please make the context value something like 'invalidWebApp' or 'invalidAppService'

Lol, yeah, my bad... copy/paste got me. Renamed to 'invalidAppService' (matches the WebAppTreeItem better).

ejizba · 2018-03-29T15:15:29Z

src/explorer/WebAppTreeItem.ts

+export class InvalidWebAppTreeItem implements IAzureTreeItem {
+    public static contextValue: string = 'file';
+    public readonly contextValue: string = InvalidWebAppTreeItem.contextValue;
+    public readonly commandId: string = 'appService.showFile';


Remove the commandId property - it was only used in the FileTreeItem

ejizba · 2018-03-29T15:18:59Z

src/explorer/WebAppTreeItem.ts

@@ -24,6 +24,7 @@ export class WebAppTreeItem extends SiteTreeItem {
    public readonly appSettingsNode: IAzureTreeItem;
    public readonly webJobsNode: IAzureTreeItem;
    public readonly folderNode: IAzureTreeItem;
+    public readonly invalidAppServiceNode: IAzureTreeItem;


Remove this property - its never used

ejizba · 2018-03-29T15:20:12Z

src/explorer/WebAppProvider.ts

-                } catch (err) {
-                    return false;
+                    const siteClient: SiteClient = new SiteClient(s, node);
+                    const appServicePlan: AppServicePlan = await siteClient.getAppServicePlan();


Wrap getAppServicePlan and treeItems.push in if (!siteClient.isFunctionApp)

We have a completely separate extension that handles function apps: https://github.com/Microsoft/vscode-azurefunctions

Ugh, fixed... forgot about that.

ejizba · 2018-03-29T15:33:10Z

src/explorer/WebAppProvider.ts

+                    const appServicePlan: AppServicePlan = await siteClient.getAppServicePlan();
+                    treeItems.push(new WebAppTreeItem(siteClient, appServicePlan));
+                } catch {
+                    treeItems.push(new InvalidWebAppTreeItem(`${s.name} (invalid)`));


I like 'invalid'. However, rather than doing the formatting yourself, please specify s.name as the label property and Invalid as the description property

That will ensure its displayed in the same format as our other description properties (which as of today is the same format that you did, but could change in the future)

Description property added.

ejizba · 2018-03-29T15:39:28Z

src/explorer/WebAppTreeItem.ts

+    public readonly contextValue: string = InvalidWebAppTreeItem.contextValue;
+    public readonly commandId: string = 'appService.showFile';
+
+    constructor(readonly label: string) {


If you didn't know, the contextValue is used to determine which commands are shown in the context menu. See the 'viewItem' property in package.json:
https://github.com/Microsoft/vscode-azureappservice/blob/master/package.json#L294

If you want to support OpenInPortal, just do the same thing in package.json except use this treeItem's contextValue as the viewItem instead of 'appService'. You also have to specify the id property on this treeItem, which is used under the covers here for openInPortal. I don't think you'll have to completely construct the fake Site object - it should just be Site.id (which should probably exist even if serverFarmId does not - but idk)

Yeah, site.id doesn't exist for these either, so that's not going to work. I'd love for it to just open to the "WebApps" blade in the portal, but I'm not sure how to get a direct link for that (when I go there in the portal, my tenant id is in the URL).

I wouldn't worry about it then. Judging by these screenshots, the Portal wasn't that helpful anyways: #370 (comment)

Roger that!

- Moved InvalideWebAppTreeItem to it's own file; - Added desciption property; - Cleaned up unused vars;

ejizba · 2018-03-29T16:57:52Z

src/explorer/InvalidWebAppTreeItem.ts

@@ -0,0 +1,25 @@
+import * as path from 'path';


Please add the Microsoft copyright header - I'm not an expert on the legalese but you can check out this section for more info: https://github.com/Microsoft/vscode-azureappservice#contributing

ejizba · 2018-03-29T16:58:08Z

src/explorer/InvalidWebAppTreeItem.ts

+import { } from 'vscode-azureappservice';
+import { IAzureTreeItem } from 'vscode-azureextensionui';
+import { } from '../constants';
+import { } from './DeploymentSlotsTreeItem';


Just remove the import lines without any variables

ejizba · 2018-03-29T17:01:36Z

src/extension.ts

@@ -214,6 +215,13 @@ export function activate(context: vscode.ExtensionContext): void {

        node.treeItem.openCdInPortal(node);
    });
+    actionHandler.registerCommand('invalidAppService.Invalid', async (node?: IAzureNode<InvalidWebAppTreeItem>) => {


What's the purpose of this command? Is it just to show the message? I think this would be confusing for users - a context menu command isn't the best place to show messages. Users expect context menu commands to actually do something

I don't think the message is necessary, but I can help you show the message as a child of the invalidAppService if you want

Yeah, I get what you mean. Went ahead and removed it... I don't think any further indicator is necessary :)

ejizba

Awesome thanks!

sunmorgus added 6 commits March 27, 2018 22:14

Fix for when SiteClient constructor throws ArgumentError;

91ecdd0

Update to show invalid resources in the explorer list as well;

0823608

Added InvalidTreeItem;

fbe4026

Merge from master

b7e8c39

Conflict resolution

31479de

Remove unused variables

8ff617e

ejizba reviewed Mar 28, 2018

View reviewed changes

sunmorgus added 2 commits March 29, 2018 08:12

Make invalid WebApps parent level items;

1ad2fee

Merge from master;

66402a2

ejizba reviewed Mar 29, 2018

View reviewed changes

- Added context menu message;

60aecef

- Moved InvalideWebAppTreeItem to it's own file; - Added desciption property; - Cleaned up unused vars;

ejizba reviewed Mar 29, 2018

View reviewed changes

sunmorgus added 3 commits March 29, 2018 15:42

Removed context menu;

43e0b77

Added microsoft copyright header;

975cdf0

Removed unused import;

6e412d9

ejizba approved these changes Mar 29, 2018

View reviewed changes

ejizba merged commit 27bcdd6 into microsoft:master Mar 29, 2018

ejizba mentioned this pull request May 10, 2018

An invalid Database Account should not prevent valid Accounts from being displayed microsoft/vscode-cosmosdb#628

Closed

github-actions bot locked and limited conversation to collaborators Feb 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show labels for invalid app services #373

Show labels for invalid app services #373

sunmorgus commented Mar 28, 2018

ejizba left a comment •

edited

Loading

sunmorgus commented Mar 29, 2018

ejizba left a comment

ejizba Mar 29, 2018

sunmorgus Mar 29, 2018

ejizba Mar 29, 2018

sunmorgus Mar 29, 2018

ejizba Mar 29, 2018

sunmorgus Mar 29, 2018

ejizba Mar 29, 2018

sunmorgus Mar 29, 2018

ejizba Mar 29, 2018

sunmorgus Mar 29, 2018

ejizba Mar 29, 2018

sunmorgus Mar 29, 2018

ejizba Mar 29, 2018

sunmorgus Mar 29, 2018

ejizba Mar 29, 2018

sunmorgus Mar 29, 2018

ejizba Mar 29, 2018

sunmorgus Mar 29, 2018

ejizba Mar 29, 2018

sunmorgus Mar 29, 2018

ejizba Mar 29, 2018

sunmorgus Mar 29, 2018

ejizba Mar 29, 2018

sunmorgus Mar 29, 2018

ejizba left a comment

Show labels for invalid app services #373

Show labels for invalid app services #373

Conversation

sunmorgus commented Mar 28, 2018

ejizba left a comment • edited Loading

Choose a reason for hiding this comment

sunmorgus commented Mar 29, 2018

ejizba 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

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

ejizba left a comment

Choose a reason for hiding this comment

ejizba left a comment •

edited

Loading