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.

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 Service Connector #2511

Merged
merged 12 commits into from
Aug 23, 2023
Merged

Add Service Connector #2511

merged 12 commits into from
Aug 23, 2023

Conversation

motm32
Copy link
Contributor

@motm32 motm32 commented Jun 9, 2023

Related to microsoft/vscode-azuretools#1419

Also includes a utils update (needed for Service Connector) and some changes to fix errors that came with the update.

@motm32 motm32 requested a review from a team as a code owner June 9, 2023 21:46
@alexweininger
Copy link
Member

Also includes a utils update (needed for Service Connector) and some changes to fix errors that came with the update.

If it's not too much work, it would be great to just split those out into a new PR. We will be able to merge that sooner than the service connector changes.

@motm32
Copy link
Contributor Author

motm32 commented Jun 9, 2023

If it's not too much work, it would be great to just split those out into a new PR. We will be able to merge that sooner than the service connector changes.

Will do!

@@ -56,9 +59,13 @@ export function registerCommands(): void {
registerCommandWithTreeNodeUnwrapping('appService.appSettings.ToggleSlotSetting', toggleSlotSetting);
registerCommandWithTreeNodeUnwrapping('appService.appSettings.Upload', uploadAppSettings);
registerCommandWithTreeNodeUnwrapping('appService.Browse', browseWebsite);
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
//@ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these @ts-ignore's needed? What was the error?

Copy link
Member

Choose a reason for hiding this comment

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

Pretty sure it's related to this microsoft/vscode-azuretools#1492

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. Let's get microsoft/vscode-azuretools#1492 fixed instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup! Once this PR (#2512) is merged I'll pull in the changes so the utils stuff is separate also.

@alexweininger
Copy link
Member

Looks good to go, just need to release the package

@motm32
Copy link
Contributor Author

motm32 commented Jul 20, 2023

The previous commit includes hooking up the UI and some changes made in the tools package to make the UI work correctly. I am not totally sure if this is the best approach so any feedback would be great.

item ??= await pickWebApp(context);
if (item instanceof SiteTreeItem) {
item = <ServiceConnectorGroupTreeItem>item.parent;
Copy link
Member

Choose a reason for hiding this comment

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

This seems backwards to me.

If it was a SiteTreeItem, would its parent be the subscription node?
But the ServiceConnectorGroupTreeItem's parent would be the SiteTreeItem, right?

Copy link
Contributor Author

@motm32 motm32 Jul 26, 2023

Choose a reason for hiding this comment

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

That is true it wasn't causing any problems for some reason since the type of item when using the command palette was not a SiteTreeItem so it was never calling that line.

I'll flip it but with that implementation I don't thing resourceId will be needed anymore.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I believe that you're right. I know I just approved it, but we could probably disregard the resourceId added in this PR

nturinski
nturinski previously approved these changes Aug 2, 2023
package.nls.json Outdated
@@ -62,6 +64,7 @@
"appService.showSavePrompt": "Show warning dialog on remote file uploading.",
"appService.startStreamingLogs": "Start Streaming Logs",
"appService.toggleAppSettingVisibility": "Toggle App Value Visibility",
"appService.validateServiceConnector": "Validate Service Connector...",
Copy link
Member

Choose a reason for hiding this comment

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

Since there are no additional prompts, the ellipses should be removed.

image

alexweininger
alexweininger previously approved these changes Aug 23, 2023
const activityContext = {
...context,
...await createActivityContext(),
activityTitle: localize('validate', `Validate {0}`, serviceConnectorName),
Copy link
Member

Choose a reason for hiding this comment

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

Last thing I promise

Suggested change
activityTitle: localize('validate', `Validate {0}`, serviceConnectorName),
activityTitle: localize('validate', `Validate connection "{0}"`, serviceConnectorName),

@motm32 motm32 merged commit 64252ce into main Aug 23, 2023
@motm32 motm32 deleted the meganmott/serviceConnector branch August 23, 2023 17:32
@microsoft microsoft locked and limited conversation to collaborators Oct 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants