Connections & Connection Groups can now be defined at both the User and Workspace level

[samgusick]

Pull Request Template – vscode-mssql

Description

This pull request fixes #19780

For those Interested in testing this branch, I've started doing releases here!

Problem:
Connections and connection groups cannot be defined at the workspace level (at least not in a user friendly, safe way). Having connections specific to workspaces can help with organization, especially when there are many connections across different projects and only a handful need to be used in each workspace.

What this does:

WARNING: these changes fundamentally alter the extensions approach to connections and connection groups within the existing framework. Many tests will need to be rewritten, and I'll need to do more work to make sure ones that should remain still pass. Additionally, user and workspace settings will be modified, so be mindful when testing with this branch.

• Instead of all connections/connection groups falling under ROOT, there will now be a "Workspace Connections" and "User Connections" group defined, with the legacy connections and their existing hierarchy migrated automatically to User Connections after upgrade.
• New connections are defined under these two new groups, or any of their child user made connection groups which the user can select in the connection dialog
• connections and connection groups defined in the workspace connection group will only be remembered at the workspace level. functionality for the User connections remains the same.
• When creating new connection groups, there's now a dropdown to decide whether the connection group will be defined at the user or workspace level.

My TO DO:

• Right now workspace settings are being created in every single folder opened with vs code on boot. If there are no workspace settings, the user should have to confirm they want workspace settings before being able to add workspace connections
• User and Workspace Connection folders should use more user friendly icons, differentiating them from regular connection folders.

Code Changes Checklist

• New or updated unit tests added
• All existing tests pass (npm run test)
• Code follows contributing guidelines
• Telemetry/logging updated if relevant
• No regressions or UX breakage

Reviewers: Please read our reviewer guidelines

[Benjin]

I'd like to take a different approach, and am interested in your feedback. What if, instead, the root ID is hardcoded as ROOT_NODE_ID?

There'd no longer be an explicit entry needed for the root in the JSON, and both workspace and user config entries could reference ROOT_NODE_ID without worry. If a user wants to create a user-json group called "User Connections" or a workspace-json group called "Workspace Connections", they can do that, both of which would have parentId: "ROOT_NODE_ID". This would also allow for shared workspace files to define connections without having to worry about syncing ROOT guids as they currently would.

What do you think?

[samgusick]

@Benjin I definitely like the flexibility that could offer, I'll give it a go. I think for that approach there will need to be some indicators in in the connections UI (visual/text based) distinguishing workspace connections from user ones. Or potentially splitting the connections window in half in some respect to create an even more clear separation, though I think there's more flexibility and less modifications necessary with the former.

[samgusick]

@Benjin after some experimenting, I think I've decided on a hybrid approach of mine and yours. We can switch to a hard coded root for the purposes you described, but I'd still like to keep workspace and user connections in separate predefined groups. Allowing users to mix and match user/workspace connections and groups just seems a bit disorganized and could create a lot of unexpected and problematic behavior that's more trouble than it's worth accounting for.

[samgusick]

@Benjin I think creating a hard coded root is out of scope for this pull request, though it shouldn't be very difficult with this new approach. That being said, all of my intended functionality is now implemented. While many tests still don't pass (and I need to rework some code to use constants and locConstants), I'd appreciate a review just to make sure I'm on the right track here and it's worth continuing on to fixing the unit tests.

[samgusick]

@aasimkhan30 @Benjin I've been consistently rebasing this branch on your changes, I've started doing releases with this branch for easier testing. Let me know any thoughts/what might need to be done beyond reworking tests to get this pull request merged.

[Benjin]

Hey @samgusick, I think we're likely going to end up making some further changes in this space internally, so while we're still looking to resolve this issue, I'm not sure if we'll end up using this specific PR. Because the final design or implementation details may end up being different from how this PR works, I worry it could cause more confusion to other users if you're posting your own releases. Also, it's important to note that we can only support the official, first-party releases that we post on the official page.

Though I also want to reiterate that we do appreciate the passion and contributions, even in instances where we don't merge the PRs in. :)