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

When canceling a deployment I immediately get a Last Deployment Successful message #2179

Closed
kgartland-rstudio opened this issue Aug 26, 2024 · 7 comments
Labels
bug Something isn't working discovery Additional discovery is necessary to determine a solution

Comments

@kgartland-rstudio
Copy link
Collaborator

kgartland-rstudio commented Aug 26, 2024

The Last Deployment Successful message is displayed immediately after a deployment is canceled.

cancel.mp4
@kgartland-rstudio kgartland-rstudio added the bug Something isn't working label Aug 26, 2024
@dotNomad dotNomad added retest design discovery Additional discovery is necessary to determine a solution and removed design labels Nov 13, 2024
@dotNomad
Copy link
Collaborator

We can use the same pattern we use for Secrets where we use the Connect API to get the status of the last deployment / publish time. We need to investigate if there is a good API endpoint for us to use for that, and what the work would entail.

We should double check that a deployment failure isn't hidden by a cancellation.

@dotNomad
Copy link
Collaborator

The issue here is that cancellation doesn't actually cancel the deployment - it merely dismisses it from the sidebar and closes the VS Code notification.

Any deployment with a cancelled local ID has its events / callbacks suppressed but several events don't contain a localId to compare.

So what occurs is the deployment file can be written when we hit the "Create Deployment Record" step, but the actual failure could occur later - in the case of a failure happening during the validate step. Because the in progress deployment is dismissed and we are showing the "Last Deployment" status in the sidebar it shows every status as the deployment file is written.

To reproduce this you can create a deployment that deploys with no issue, but has an error when running. For example: having a typo on the fastapi import:

from fastapi import FstAPI

Deploying writes a valid deployment record, until validation occurs then we get:

[deployment_error]
code = 'deployedContentNotRunning'
message = 'Deployed content does not seem to be running. See the logs in Connect for details.'
operation = 'publish/validateDeployment'

@dotNomad dotNomad removed the retest label Nov 19, 2024
@dotNomad dotNomad assigned dotNomad and unassigned dotNomad Nov 19, 2024
@dotNomad
Copy link
Collaborator

Currently we write the deployment record (via writeDeploymentRecord):

Because we keep the deployment record up-to-date throughout the deploy process, showing the "last deployed state" while a deploy is occurring will cause confusion.

Since VS Code can be stopped at anytime maintaining an "in-progress" state inside of the deployment record file would be inadvisable.

@jonkeane
Copy link
Collaborator

Any deployment with a cancelled local ID has its events / callbacks suppressed but several events don't contain a localId to compare.

So what occurs is the deployment file can be written when we hit the "Create Deployment Record" step, but the actual failure could occur later - in the case of a failure happening during the validate step. Because the in progress deployment is dismissed and we are showing the "Last Deployment" status in the sidebar it shows every status as the deployment file is written.

Because we keep the deployment record up-to-date throughout the deploy process, showing the "last deployed state" while a deploy is occurring will cause confusion.

Since VS Code can be stopped at anytime maintaining an "in-progress" state inside of the deployment record file would be inadvisable.

I wonder if this is telling us that maintaining state like this in the deployment file + using that to drive (some of, and only sometimes?) the UI isn't helpful for us? Maybe an approach where the status isn't kept locally in a file but actually looks at the server would be better? That has it's own tradeoffs, and we would need at least some cache/local state even if it's "just" what's in the DOM. But if what we have here makes it hard for us to not reflect confusing states, we should think if there's a better approach we could take that makes it easier for us.

Or maybe that we need to change up the way we're doing that? For one example: if we had localId on all of the events / callbacks would that make cancel more of an actual cancel (at least locally, I fully understand that on the connect server it's not and that we aren't tackling that right now)?

@dotNomad
Copy link
Collaborator

Maybe an approach where the status isn't kept locally in a file but actually looks at the server would be better?

This is exactly what I was thinking. I will dig into the Connect API docs to see if there is a good option for getting the status we'd like to display.

@dotNomad
Copy link
Collaborator

Looking over the Connect APIs available, we cannot exclusively rely on them for the information we'd like to present.

  • GET /v1/content/{guid}

    • contains last_deployed_time but the description mentions "The timestamp (RFC3339) indicating when this content last had a successful bundle deployment performed."
    • It doesn't contain last error information we get from the event stream
  • GET /v1/content/{guid}/bundles/{id}

    • doesn't contain error information either

We also want to present our pre-check failures which never hit Connect - for example: setting a Python version that isn't on the server.


Based on what we currently show in the deployment status section of the sidebar I think the best option would be to respect that something is still being deployed.

Rather than attempting to hide the current deployment in progress when "Cancel" is pressed we can instead show "Deployment in Progress...". Keeping that displayed would prevent the confusing statuses from Deployment record updates, and would better reflect what is occurring in Publisher.

The reason we introduced the cancel was to address hung deployments (#2057): we can still address that by removing our disabling of the "Deploy Your Project" button when a deployment is in progress.

@dotNomad dotNomad removed their assignment Dec 9, 2024
@dotNomad dotNomad modified the milestones: v1.6.0, v.1.8.0 Dec 9, 2024
@m--
Copy link
Collaborator

m-- commented Dec 17, 2024

#2498 will remove the need for this one. Closing this in favor of that, for the time being.

@m-- m-- closed this as completed Dec 17, 2024
@m-- m-- removed this from the v1.8.0 milestone Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working discovery Additional discovery is necessary to determine a solution
Projects
None yet
Development

No branches or pull requests

4 participants