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

Listen for and show publish failure events in progress view #321

Merged
merged 6 commits into from
Nov 2, 2023

Conversation

dotNomad
Copy link
Collaborator

This PR makes each step in the publish process subscribe to its correlated /failure server side event. It uses the same msg.data.message extraction and places that inside of the logs passed to PublishStep.vue which styles error logs and the surrounding expandable section.

Preview

CleanShot 2023-10-26 at 14 15 57

Intent

Resolves #302

Type of Change

  • Bug Fix
  • New Feature
  • Breaking Change

Approach

My approach here was to make the generic component PublishStep.vue style itself differently if it received a log message that was an error. If it contains an error message it hasError = true which changes the icon, color, and stylizes the error to be semi-bold (medium weight) and red as well.

Some TypeScript changes (types and type predicate functions) were needed to distinguish from a generic string log message and an AdvancedLog message which kept the usage of PublishStep.vue as simple as possible.

A lot of the changes in the publish step components is the same other than the event they specifically subscribe to.

Directions for Reviewers

Create an error when publishing to see the error get handled. The easiest example that I used was pointing to a non-existent destination like a stopped localhost:3939.

Keep in mind we cannot cancel a publish, and an error message doesn't currently set publishInProgress = false to allow the usage of the back button. Of course we can change that in another PR or here based on feedback.

Copy link
Collaborator

@sagerb sagerb left a comment

Choose a reason for hiding this comment

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

Just a couple of nits.

Nice and simple overall.

@@ -22,26 +26,46 @@
:key="index"
>
<q-item-section>
{{ log }}
<template v-if="isErrorLog(log)">
<span class="text-red text-weight-medium">{{ log.msg }}</span>
Copy link
Collaborator

Choose a reason for hiding this comment

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

The specification of the actual color should come from the color store as a computed value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh I assumed that we would use the color store if the color wasn't consistently the same. I will switch this over to using the store.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed in 66f712d

I kept it pretty generic colorStore.activePallete.textError so the error text wasn't specific to the part of the UI we were in. Let me know if you had something else in mind.

import { useColorStore } from 'src/stores/color';
import { colorToHex } from 'src/utils/colorValues';

type AdvancedLog = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps we should move Log type definition outside of PublishStep, and have it located with our other API types?

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 personally comfortable having this type in the PublishStep component for it to specify for itself what it takes a log for its logs prop.

If this gets re-used in multiple components then I think it should be extracted and shared.

However to touch on your question if it should be located with our other API types - I don't think any type that isn't related to the API responses, API requests, or how we interact with them should be located in the api module.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree it should, but in this case, aren't you simply replicating exactly what was being sent in on the event message? So I would think it would own the type unless it was changed by the prop? This is where my observation was coming from...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After a discussion we agreed that I am replicating the event msg structure and we can pass that directly to PublishStep without much issue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed in 79fdf6e

@dotNomad dotNomad requested a review from sagerb November 1, 2023 23:14
Copy link
Collaborator

@sagerb sagerb left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! LGTM!

@dotNomad dotNomad merged commit 6feb329 into main Nov 2, 2023
17 checks passed
@dotNomad dotNomad deleted the dotnomad/publish-errors branch November 2, 2023 18:24
@kgartland-rstudio
Copy link
Collaborator

Failure scenarios tested:

Create Bundle:
✅ Deploy a non-existant file

Create Deployment:
✅ Deploy to an invalid server

Restore Python Environment:
✅ Deploy while omitting a requirements.txt file
✅ Deploy to a server with mismatching python version

Run Content:
❌ Deploy with a bad import statement

I attempted to give the app a bad import statement and expected this step to fail since it fails in Connect.

How is the Run Content step validating that we're able to run content?

Recently in rsconnect-python @mmarchetti included a Verifying deployed content step, should that same logic be applied here? LMK if this should be a separate issue.

@mmarchetti
Copy link
Contributor

LMK if this should be a separate issue

Verifying app deployments are live should be a separate issue. For now, you could test having a bad import in a jupyter notebook, which will fail rendering and therefore the deployment.

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.

Handle publish failures in progress view
4 participants