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 #3790

Merged
merged 7 commits into from
Aug 29, 2023
Merged

Add Service Connector #3790

merged 7 commits into from
Aug 29, 2023

Conversation

motm32
Copy link
Contributor

@motm32 motm32 commented Aug 8, 2023

Service connector is now available on the portal for function apps.

Related to microsoft/vscode-azuretools#1419

Here is how it looks in the tree view:
image

@motm32 motm32 requested a review from a team as a code owner August 8, 2023 21:43
package.json Outdated
Comment on lines 639 to 653
},
{
"command": "azureFunctions.createServiceConnector",
"when": "view == azureResourceGroups && viewItem =~ /serviceConnectorGroupItem/",
"group": "4@1"
},
{
"command": "azureFunctions.deleteServiceConnector",
"when": "view == azureResourceGroups && viewItem =~ /serviceConnectorItem/",
"group": "4@2"
},
{
"command": "azureFunctions.validateServiceConnector",
"when": "view == azureResourceGroups && viewItem =~ /serviceConnectorItem/",
"group": "4@3"
Copy link
Member

Choose a reason for hiding this comment

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

These need to be available in the focus view also

package.nls.json Outdated
@@ -101,6 +103,7 @@
"azureFunctions.toggleAppSettingVisibility": "Toggle App Setting Visibility.",
"azureFunctions.uninstallFuncCoreTools": "Uninstall Azure Functions Core Tools",
"azureFunctions.validateFuncCoreTools": "Validate the Azure Functions Core Tools is installed before debugging.",
"azureFunctions.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.

Is this the right wording? Should it be "Validate connection" or "Validate"?

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'm not sure what it says on the Portal, but I feel like Validate connection makes more sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now the portal just says Validate. I could also say Validate connection status since that is also shown in the portal
image

const activityContext = {
...context,
...await createActivityContext(),
activityTitle: localize('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.

Depending on what you do with Alex's comment above, make sure that you keep it consistent here.

@@ -22,7 +22,7 @@ export async function validateServiceConnector(context: IActionContext, item?: S
const activityContext = {
...context,
...await createActivityContext(),
activityTitle: localize('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.

I think we want this title to be more detailed. Is it possible to add in the name of what we're validating here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like add in the name of the specific 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.

Yeah, I feel that the activity title should have more detail so it's easy to know what is being validated.

Copy link
Member

Choose a reason for hiding this comment

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

I think at the very least it can be changed back to "Validate Service Connector"

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 think adding the name of the specific service connector makes the most sense!

Copy link
Member

Choose a reason for hiding this comment

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

image

When I do it from the command palette it says Validate undefined

Copy link
Member

Choose a reason for hiding this comment

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

I think I'd be OK just hiding this from the command palette.

Copy link
Member

@alexweininger alexweininger left a comment

Choose a reason for hiding this comment

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

Also if I validate twice, the activity children don't have unique ids and it throws an error.

image

@motm32
Copy link
Contributor Author

motm32 commented Aug 21, 2023

Also if I validate twice, the activity children don't have unique ids and it throws an error.

Hmm how did you do this? I validated the same connection twice from both the tree and command palette and they both succeeded.

@alexweininger
Copy link
Member

Also if I validate twice, the activity children don't have unique ids and it throws an error.

Hmm how did you do this? I validated the same connection twice from both the tree and command palette and they both succeeded.

Happens when I expand both activities and reveal the children

@MicroFish91
Copy link
Contributor

MicroFish91 commented Aug 22, 2023

Also if I validate twice, the activity children don't have unique ids and it throws an error.

Hmm how did you do this? I validated the same connection twice from both the tree and command palette and they both succeeded.

Yeah I noticed this happening to me in Container Apps as well. My workaround has been to incorporate random UUIDs to keep the contexts original.

nturinski
nturinski previously approved these changes Aug 23, 2023
@@ -101,6 +103,7 @@
"azureFunctions.toggleAppSettingVisibility": "Toggle App Setting Visibility.",
"azureFunctions.uninstallFuncCoreTools": "Uninstall Azure Functions Core Tools",
"azureFunctions.validateFuncCoreTools": "Validate the Azure Functions Core Tools is installed before debugging.",
"azureFunctions.validateServiceConnector": "Validate",
Copy link
Member

Choose a reason for hiding this comment

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

Make sure to make the same changes you did on the App Service PR here

Copy link
Member

Choose a reason for hiding this comment

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

nvm you got it

alexweininger
alexweininger previously approved these changes Aug 23, 2023
@motm32 motm32 dismissed stale reviews from alexweininger and nturinski via 404d7ef August 29, 2023 17:54
@motm32 motm32 merged commit 58588c1 into main Aug 29, 2023
@motm32 motm32 deleted the meganmott/serviceConnector branch August 29, 2023 20:17
nturinski added a commit that referenced this pull request Sep 17, 2023
nturinski added a commit that referenced this pull request Sep 19, 2023
…3848)

* Revert node v4 changes

* Revert "Add Service Connector (#3790)"

This reverts commit 58588c1.

* npm install
@microsoft microsoft locked and limited conversation to collaborators Oct 14, 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