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 ability to update the target guid for a deployment at any time (from the ... menu) #2576

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

sagerb
Copy link
Collaborator

@sagerb sagerb commented Jan 30, 2025

…rom ... menu)

Intent

Add the ability to associate an existing deployment with a different GUID. This simply offers the same capability that we had previously offered ahead of the first deployment within the UX. This option is now always available via the ... menu when a deployment has been selected.

Resolves #2400

Type of Change

    • Bug Fix
    • New Feature
    • Breaking Change
    • Documentation
    • Refactor
    • Tooling

Approach

I started by simply exposing existing functionality into the context menu and adding a success notification.

I then realized that the menu item doesn't make any sense if there is no credential match, so I've added and updated a context within the extension that can be used to show or hide the menu item.

New screenshots:

When there are credentials to the server and the active selection has been deployed:
image

When there are no credentials to the server:
image

When the active selection has not yet been deployed:
image

It will show when other errors w/ the config are present:
image

I also added an informational message when it has completed calling the patch API.
2025-01-30 at 10 32 AM

User Impact

New feature!

@sagerb sagerb self-assigned this Jan 30, 2025
Comment on lines 186 to 191
hostConduit.sendMsg({
kind: WebviewToHostMessageType.UPDATE_SELECTION_CREDENTIAL_STATE,
content: {
state: serverCredential !== undefined ? "true" : "false",
},
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is always sending back true. Looks like it might be a missing .value on serverCredential.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand the purpose of the message sending however. This is checking if the current deployment has a credential, but if we are associating with a different piece of content we could be pointing to an entirely different server. What is this supposed to be catching?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The purpose is to enable or disable the ability to associate, within the ... menu. I've seen it work, so I'm surprised there is an error there, but I'll check it out. What it implements is that when we do not have a credential for the server, so we couldn't contact the server to patch the deployment file, so it would fail always. So the option isn't offered.

I'll check the logic, like I mentioned, I'm not sure with this how I was seeing the vscode context set correctly per the state, as shown in the screen shots.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated to examine the reference's value rather than the reference itself. Thanks for catching this. The message stayed in place, as my explanation of its purpose above still stands.

@@ -325,6 +330,10 @@
"command": "posit.publisher.homeView.showSelectConfigForDeployment",
"when": "webviewId == 'posit.publisher.homeView' && webviewSection == 'even-easier-deploy-more-menu-matching-configs'"
},
{
"command": "posit.publisher.homeView.showAssociateDeployment",
"when": "webviewId == 'posit.publisher.homeView' && webviewSection == 'even-easier-deploy-more-menu-matching-configs' && posit.publish.selection.haveCredentialMatch == 'true'"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This shows the context menu option while a pre-deployment shows the association link.

We could try to hide it using message passing or we could generalize the label to remove the "with a Different Deployment". 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not following you. Can you provide a screenshot? I'm looking at
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah that screenshot shows what I'm talking about. We have both "update that previous deployment" available as a link, and the "Associate with a Different Deployment on Connect". The wording is just strange since it isn't associated with any deployment yet. That may be too in the weeds with how this is working though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've added another context state within the extension, which indicates if the actively selected content record is a PreContentRecord or not. This allows the menu item to now be shown only when it has been deployed, which should address the confusion you identified here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for doing that, it is a small thing, but it should make it easier to understand for users.

@sagerb sagerb requested a review from dotNomad February 10, 2025 18:32
@jonkeane jonkeane changed the title Add ability to update the target quid for a deployment at any time (from the ... menu) Add ability to update the target guid for a deployment at any time (from the ... menu) Feb 10, 2025
@jonkeane
Copy link
Collaborator

I just s/quid/guid/ on the title cause I thought that was a typo, but if it's not sorry!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to point deployment to a different content GUID
3 participants