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

Remove disable states for the Deploy button #2344

Open
dotNomad opened this issue Oct 3, 2024 · 2 comments
Open

Remove disable states for the Deploy button #2344

dotNomad opened this issue Oct 3, 2024 · 2 comments

Comments

@dotNomad
Copy link
Collaborator

dotNomad commented Oct 3, 2024

We currently have 3 disabled states for the Deploy button in the extension:

  1. Resources haven't been selected
  2. The selected Configuration is in error
  3. There is currently a deploy in progress

1. Resources haven't been selected

We do not show the deploy button at all when a Content Record has not been selected.

The deploy button is shown if we have a record selected, but can still be disabled in the two possible states past that:

  • the user doesn't have a Credential for the Content Record
  • the user doesn't have a Configuration for the Content Record

For both states we show warnings to solve this easily.

We cannot easily remove this since the deploy API in our Go binary requires we pass the name of the both the account (Credential) and the name of the Configuration.

A decent amount of refactoring could remove the need of requiring a Credential in that API call since it can be derived. Credentials are unique to server URLs and we can attempt to find that Credential during the deploy process and fail if it is not found. This is how the extension is determining the "selected Credential" despite the user never making that selection explicit:

const serverCredential = computed(() => {

Removal of the Configuration disabled state could be done in much the same way. We can rely on the selected Content Record to point to a Configuration since that is how we determine the selection in the extension:

const selectedConfiguration = computed(

The path forward to remove these disabled states would be to refactor the POST /api//deployments/$NAME to not take a account (Credential) or config and derive them on the Go side.

This would free up the extension to always deploy when a selection of a Content Record is made, and allow errors to bubble up with the derivation goes wrong.


2. Selected Configuration is in error

We can make the deploy API call to our binary just fine with a Configuration in error since we still have the name of the Configuration.

It will not ever successfully deploy since we validate the TOML file before sending anything up to Connect:

func (v *Validator[T]) ValidateTOMLFile(path util.AbsolutePath) error {

This validation is necessary to get the attributes from a Configuration, but:

  • we can allow the pressing of Deploy
  • the user can get an informative error from our ValidateTOMLFile function

That looks like the below, when the notification is expanded:

Image


3. Deploy in progress

Allowing for deploy while another is in progress is possible, but leads to some odd states.

First it allows users to accidentally double click the button sending up two deploys. We would have to introduce some sort of debouncing or something else to avoid this.

Second we start to get errors where builds failed due to being superseded by a more recent deployment.

Image

There are questions about how we handle this in our current UX when it comes to logging. Currently our Posit Publisher Logs only track the latest deployment. If multiple deployments are going at once you are either:

  • only seeing the logs for the most recent
  • seeing multiple logs at once (multiple steps in progress) if you are deploying multiple of the same content ID twice

Even if we get into a bad state where publishInProgress remains true (for example a Deployment gets stuck) there are few options forward:

This will require more thought on how to approach removing this disable state.

@dotNomad dotNomad added discovery Additional discovery is necessary to determine a solution vscode labels Oct 3, 2024
@dotNomad dotNomad changed the title Investigate removal of Deploy disable states Remove disable states for the Deploy button Oct 3, 2024
@dotNomad dotNomad added epic and removed discovery Additional discovery is necessary to determine a solution labels Oct 3, 2024
@dotNomad
Copy link
Collaborator Author

dotNomad commented Oct 3, 2024

Ultimately I split this out into 3 sub-issues to remove the disable states. The only one that can be removed quickly is #2345 and I will have a branch up for that in a moment.

The others are a bit more difficult. It is possible to get to a point where the only disable state we have is "a Deployment has not been chosen". I don't think we can get away from that one.

@jonkeane
Copy link
Collaborator

jonkeane commented Oct 3, 2024

The others are a bit more difficult. It is possible to get to a point where the only disable state we have is "a Deployment has not been chosen". I don't think we can get away from that one.

Agreed that no deployment has been chosen is a reasonable state to disable the button.

Disabling while deploying also might be reasonable to leave as is. Certainly the state(s) one can get into by pushing that button multiple times are funky. And we do have the cancel action which lets someone get out of the disabled state if they needed to.

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

No branches or pull requests

2 participants