-
Notifications
You must be signed in to change notification settings - Fork 906
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 support for custom endpoints #23390
Conversation
Discussed offline, please consider adding a command to generate json template in user directory from where you want to read settings. And let user modify that and restart ADS. Provide these instructions in setting description so they know how to proceed. |
Is there a reason you want to have a separate config file instead of just having the user enter it into their settings.json directly? That has the added benefit of coming with automatic hover/autocomplete support in ADS. |
Co-authored-by: Cheena Malhotra <13396919+cheenamalhotra@users.noreply.github.com>
Co-authored-by: Cheena Malhotra <13396919+cheenamalhotra@users.noreply.github.com>
@@ -161,6 +166,118 @@ export async function updateTenantIgnoreList(tenantIgnoreList: string[]): Promis | |||
await configuration.update(constants.Filter, tenantIgnoreList, vscode.ConfigurationTarget.Global); | |||
} | |||
|
|||
export function updateProviderSettings(defaultSettings: ProviderSettings[]): ProviderSettings[] { | |||
let providerSettingsJson: ProviderSettingsJson[] | undefined = vscode.workspace.getConfiguration(constants.AzureSection).get(constants.ProviderSettingsJson) as ProviderSettingsJson[]; | |||
vscode.workspace.onDidChangeConfiguration(async (changeEvent) => { |
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.
How does this behavior if the user has auto-save on? Will it keep popping up the prompt every time they type a letter?
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.
it will pop up every time the file is saved yes, but the auto save setting itself will not save after every letter. The options for autosave are onFocusChange, onWindowChange, or afterDelay (which can be set by the user).
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, I would definitely suggest keeping an eye on this and having extra people test it out themselves to see if the experience is nice to use. It might be worth adding our own debounce delay to this with a longer timeout (especially if these don't take effect until after restart).
Telemetry could help here too if you wanted to add some events like how often the toast is shown...
Would also suggest updating the description with new screenshots/gifs showing the behavior - the current one is outdated. |
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'd definitely suggest adding telemetry at some point to track usage and errors. Indicating what provider is currently being used for any operation in azurecore is probably something that will be useful - especially for error telemetry - as well.
"config.providerSettingsMetadata": "Cloud Display Name", | ||
"config.providerSettingsId": "Cloud ID", | ||
"config.providerSettings.endpoints.host": "Host Endpoint", | ||
"config.providerSettings.endpoints.clientId": "Client ID for Azure Data Studio", |
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.
Will you be providing instructions on how to set this up? (and all of this really) It doesn't seem super straightforward, so having a doc about this would probably be the best.
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.
Yes I'll work on creating a doc for this
@@ -161,6 +166,118 @@ export async function updateTenantIgnoreList(tenantIgnoreList: string[]): Promis | |||
await configuration.update(constants.Filter, tenantIgnoreList, vscode.ConfigurationTarget.Global); | |||
} | |||
|
|||
export function updateProviderSettings(defaultSettings: ProviderSettings[]): ProviderSettings[] { | |||
let providerSettingsJson: ProviderSettingsJson[] | undefined = vscode.workspace.getConfiguration(constants.AzureSection).get(constants.ProviderSettingsJson) as ProviderSettingsJson[]; | |||
vscode.workspace.onDidChangeConfiguration(async (changeEvent) => { |
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, I would definitely suggest keeping an eye on this and having extra people test it out themselves to see if the experience is nice to use. It might be worth adding our own debounce delay to this with a longer timeout (especially if these don't take effect until after restart).
Telemetry could help here too if you wanted to add some events like how often the toast is shown...
Co-authored-by: Charles Gagnon <chgagnon@microsoft.com>
Co-authored-by: Charles Gagnon <chgagnon@microsoft.com>
Opened #23922 to track adding the telemetry, will do in a separate PR |
Added a setting where users can add custom provider endpoints to the settings.json file, where they will be picked up and added as an additional provider.
Also shows a notification message once the custom endpoints file has been successfully loaded:
This shows the behavior if the user is missing required fields in their settings.json file:
This shows the account dialog once the new cloud has been added:
I tested by removing the azure public cloud endpoints from the providerSettings file and added them back in via the JSON file and everything worked correctly, but more testing always helps if you have time.
Fixes #23205