Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show labels for invalid app services #373

Merged
merged 12 commits into from
Mar 29, 2018
Merged

Show labels for invalid app services #373

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
91ecdd0
Fix for when SiteClient constructor throws ArgumentError;
sunmorgus Mar 28, 2018
0823608
Update to show invalid resources in the explorer list as well;
sunmorgus Mar 28, 2018
fbe4026
Added InvalidTreeItem;
sunmorgus Mar 28, 2018
b7e8c39
Merge from master
sunmorgus Mar 28, 2018
31479de
Conflict resolution
sunmorgus Mar 28, 2018
8ff617e
Remove unused variables
sunmorgus Mar 28, 2018
1ad2fee
Make invalid WebApps parent level items;
sunmorgus Mar 29, 2018
66402a2
Merge from master;
sunmorgus Mar 29, 2018
60aecef
- Added context menu message;
sunmorgus Mar 29, 2018
43e0b77
Removed context menu;
sunmorgus Mar 29, 2018
975cdf0
Added microsoft copyright header;
sunmorgus Mar 29, 2018
6e412d9
Removed unused import;
sunmorgus Mar 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions resources/dark/WebApp_grayscale.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
7 changes: 7 additions & 0 deletions resources/light/WebApp_grayscale.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
24 changes: 10 additions & 14 deletions src/explorer/WebAppProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { AppServicePlan, Site, WebAppCollection } from 'azure-arm-website/lib/mo
import { createWebApp, SiteClient } from 'vscode-azureappservice';
import { IActionContext, IAzureNode, IAzureTreeItem, IChildProvider, UserCancelledError } from 'vscode-azureextensionui';
import * as util from '../util';
import { WebAppTreeItem } from './WebAppTreeItem';
import { InvalidWebAppTreeItem, WebAppTreeItem } from './WebAppTreeItem';

export class WebAppProvider implements IChildProvider {
public readonly childTypeLabel: string = 'Web App';
Expand All @@ -31,22 +31,18 @@ export class WebAppProvider implements IChildProvider {

this._nextLink = webAppCollection.nextLink;

return await Promise.all(webAppCollection
.filter((s: Site) => {
const treeItems: IAzureTreeItem[] = [];
await Promise.all(webAppCollection
.map(async (s: Site) => {
try {
const siteClient = new SiteClient(s, node);
return siteClient !== undefined;
// tslint:disable-next-line:no-unsafe-any
} catch (err) {
return false;
const siteClient: SiteClient = new SiteClient(s, node);
const appServicePlan: AppServicePlan = await siteClient.getAppServicePlan();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, fixed... forgot about that.

treeItems.push(new WebAppTreeItem(siteClient, appServicePlan));
} catch {
treeItems.push(new InvalidWebAppTreeItem(`${s.name} (invalid)`));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Description property added.

}
})
.map((s: Site) => new SiteClient(s, node))
.filter((s: SiteClient) => !s.isFunctionApp)
.map(async (s: SiteClient) => {
const appServicePlan: AppServicePlan = await s.getAppServicePlan();
return new WebAppTreeItem(s, appServicePlan);
}));
return treeItems;
}

public async createChild(node: IAzureNode<IAzureTreeItem>, showCreatingNode: (label: string) => void, actionContext: IActionContext): Promise<IAzureTreeItem> {
Expand Down
18 changes: 18 additions & 0 deletions src/explorer/WebAppTreeItem.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this property - its never used

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed


constructor(client: SiteClient, appServicePlan: AppServicePlan) {
super(client);
Expand Down Expand Up @@ -126,3 +127,20 @@ export class WebAppTreeItem extends SiteTreeItem {
return await fs.readFile(templatePath, 'utf8');
}
}

export class InvalidWebAppTreeItem implements IAzureTreeItem {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

public static contextValue: string = 'file';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

public readonly contextValue: string = InvalidWebAppTreeItem.contextValue;
public readonly commandId: string = 'appService.showFile';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.


constructor(readonly label: string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Roger that!

}

public get iconPath(): { light: string, dark: string } {
const iconName = 'WebApp_grayscale.svg';
return {
light: path.join(__filename, '..', '..', '..', '..', 'resources', 'light', iconName),
dark: path.join(__filename, '..', '..', '..', '..', 'resources', 'dark', iconName)
};
}
}