-
Notifications
You must be signed in to change notification settings - Fork 32
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.
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
Add Tenants view #869
Add Tenants view #869
Conversation
Note for later: lets update the people in this issue once this is merged - microsoft/vscode-azure-account#901 |
Also just a note for reviewing. This includes a lot of code at the moment that is from the auth tools package for testing purposes. I will remove that code once we are closer to being done. |
@@ -73,3 +84,15 @@ export async function getSelectedTenantAndSubscriptionIds(): Promise<string[]> { | |||
async function setSelectedTenantAndSubscriptionIds(tenantAndSubscriptionIds: string[]): Promise<void> { | |||
await settingUtils.updateGlobalSetting('selectedSubscriptions', tenantAndSubscriptionIds); | |||
} | |||
|
|||
export function getTenantFilteredSubscriptions(allSubscriptions: AzureSubscription[]): AzureSubscription[] | undefined { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand what this function is doing, and why it returns undefined instead an empty array. I would either refactor or make it more clear that this actually matters later on in the UI logic.
I wouldn't want someone to refactor this based on the function name and then mess things up.
@@ -56,7 +56,7 @@ export class GroupingItem implements ResourceGroupsItem { | |||
} : undefined; | |||
|
|||
if (this.context?.subscription) { | |||
this.id = `/subscriptions/${this.context?.subscriptionContext.subscriptionId}/groupings/${this.label}`; | |||
this.id = `/subscriptions/${this.context?.subscriptionContext.subscriptionId}/account/${this.context?.subscription.account?.id}/groupings/${this.label}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we tested reveal to make sure it still works with this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not tested it but I am pretty sure based on discussions with Nathan it would break things. I'll test it out and see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, we'll need to sort that out before merging. I can help with that too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tested it out by creating a storage account and clicking "click to view resource" in the activity log and nothing happened so seems to be broken. Before releasing this we definitely want to fix that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump
One comment. Make sure it supports Lighthouse. With Lighthouse you can view the same subscriptions under different tenants. Currently it works the halfway as you can see the subscriptions twice but you can only select the same subscription once to be displayed otherwise you will get error. Also currently of course you do not know which subscription selected is under which tenant. |
Hi @slavizh thanks for pointing this out! Most of these issues should be resolved by some updates we have made recently. Before we plan to release if you would like we could send you a custom build so you can try the tenants view with Lighthouse 😊 |
|
||
// This function is also used to filter subscription tree items in AzureResourceTreeDataProvider | ||
export function getTenantFilteredSubscriptions(allSubscriptions: AzureSubscription[]): AzureSubscription[] | undefined { | ||
const tenants = ext.context.globalState.get<string[]>('unselectedTenants'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somewhere we should store this string as a constant.
@motm32 Yeah, no problem I will do some tests to verify. |
* Fixes for multi account * Random fixes and changes * Fixup * Fixup unselected tenant logic (#963) * Install latest version of auth package
Hi @slavizh. Here is the custom vsix you can use to test Lighthouse! On VS Code you can open the command prompt (ctrl + shift + P) and use the command |
package.json
Outdated
@@ -272,6 +290,11 @@ | |||
"name": "Workspace", | |||
"visibility": "visible" | |||
}, | |||
{ | |||
"id": "azureTenant", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be clearer if we changed the id to "azureTenantsView". Along with the prefixes for the associated commands.
src/extension.ts
Outdated
@@ -154,7 +173,7 @@ export async function activate(context: vscode.ExtensionContext, perfStats: { lo | |||
azureResourceTreeDataProvider, | |||
workspaceResourceProviderManager, | |||
workspaceResourceBranchDataProviderManager, | |||
workspaceResourceTreeDataProvider, | |||
workspaceResourceTreeDataProvider |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: add back the comma
src/tree/OnGetChildrenBase.ts
Outdated
children.push(new GenericItem( | ||
localize('createAccountLabel', 'Create an Azure Account...'), | ||
{ | ||
commandId: 'azureResourceGroups.openUrl', | ||
commandArgs: ['https://aka.ms/VSCodeCreateAzureAccount'], | ||
iconPath: new vscode.ThemeIcon('add') | ||
})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These blocks could be cleaned up.
children.push(
new GenericItem(
localize('createAccountLabel', 'Create an Azure Account...'),
{
commandId: 'azureResourceGroups.openUrl',
commandArgs: ['https://aka.ms/VSCodeCreateAzureAccount'],
iconPath: new vscode.ThemeIcon('add')
}
)
);
or this could work.
children.push(
new GenericItem(localize('createAccountLabel', 'Create an Azure Account...'),
{
commandId: 'azureResourceGroups.openUrl',
commandArgs: ['https://aka.ms/VSCodeCreateAzureAccount'],
iconPath: new vscode.ThemeIcon('add')
}
)
);
src/extensionVariables.ts
Outdated
@@ -32,6 +33,8 @@ export namespace ext { | |||
// TODO: do we need this? only used by load more command | |||
export let workspaceTree: AzExtTreeDataProvider; | |||
export let workspaceTreeView: TreeView<unknown>; | |||
export let tenantTree: AzExtTreeDataProvider; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove this since it's for v1 backwards compatibility and we should never need that for the tenants view.
src/commands/registerCommands.ts
Outdated
ext.actions.refreshTenantTree(node); | ||
}); | ||
|
||
registerCommand('azureTenant.signInToTenant', async (_context, node: TenantTreeItem, account?: AuthenticationSessionAccountInformation) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just following up on previous comment... here is where I think it'd be better if the command was azureTenantView.signInToTenant
import { AzureResourceTreeDataProviderBase } from "./azure/AzureResourceTreeDataProviderBase"; | ||
import { TenantResourceTreeDataProvider } from "./tenants/TenantResourceTreeDataProvider"; | ||
|
||
export async function OnGetChildrenBase(subscriptionProvider: AzureSubscriptionProvider, tdp?: AzureResourceTreeDataProvider): Promise<ResourceGroupsItem[]> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, I really like this shared approach.
return removeDuplicates(value); | ||
} | ||
|
||
export function isTenantFilteredOut(tenantId: string, accountId: string): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non blocker, but we should plan to convert this to use a setting
export class TenantTreeItem implements ResourceGroupsItem { | ||
constructor(public readonly label: string, public tenantId: string, public readonly account: vscode.AuthenticationSessionAccountInformation, private readonly options?: TenantItemOptions) { | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: maybe this was a design choice, but you could pass the whole tenant object here instead of requesting the label and tenantId here. Then you can set tenantId and label based on the tenant object in the contructor.
@@ -67,6 +67,7 @@ export class AzureResourceTreeDataProvider extends AzureResourceTreeDataProvider | |||
return await element.getChildren(); | |||
} else { | |||
const subscriptionProvider = await getAzureSubscriptionProvider(this); | |||
// When a user is signed in 'OnGetChildrenBase' will return the no children |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// When a user is signed in 'OnGetChildrenBase' will return the no children | |
// When a user is signed in 'OnGetChildrenBase' will return no children |
@motm32 I did not spot any major issues but I would like to suggest some improvements:
I hope this will be helpful. |
@motm32 ignore the last reply. I did not saw the new pane so will come back with new feedback after testing that. |
@motm32 My feedback after testing the accounts and tenant view:
This feedback that was from previous reply stays:
FYI I have over 700 subscriptions. I hope this helps. |
@slavizh Thank you so much for your feedback it is very helpful 😊 |
If possible @slavizh could you provide a number of how many tenants you had in the view? For instance were the 700 subscriptions you mentioned were belonging to one tenant or spread across multiple? |
@motm32 the subscriptions belong to multiple tenants because they are project via Lighthouse but I think in this case you are asking mostly if the subscriptions are seen from multiple tenants or from single one. In my case I have tenant where I have over 700 subscriptions, another tenant with 8 subscriptions. Those 8 are also seen on the other tenant via Lighthouse. These two tenants are under the same account. I also have my personal account where I have 2 subscriptions under one tenant. Over my personal account I also have 3 other tenants visible but there are no subscriptions for those. |
Completes #849
Here is how the view will look:
Detailed changes
Related to accounts:
Related to tenants:
Sign in to Tenant
. Finally if users check an unauthenticated tenant they will automatically be prompted to sign into that tenant.To do:
Sign out of Azure
code so the tenants view also gets refreshed