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

[cli][client] Add step for checks #6889

Merged
merged 9 commits into from
Oct 27, 2021
Merged

[cli][client] Add step for checks #6889

merged 9 commits into from
Oct 27, 2021

Conversation

AndyBitz
Copy link
Contributor

@AndyBitz AndyBitz commented Oct 26, 2021

Related Issues

https://github.com/vercel/runtimes/issues/201

Shows an error message in case the checks failed and a spinner while they're running.

It won't show any new information when there are no checks, if they're skipped, or if they all succeed, since it wouldn't really fit into the Inspect, Production/Preview view.

For the error we also don't need an extra link, because the inspect link above will be the only relevant one.

Tests

  • The code changed/added as part of this PR has been covered with tests
  • All tests pass locally with yarn test-unit

Code Review

  • This PR has a concise title and thorough description useful to a reviewer
  • Issue from task tracker has a link to this PR

@codecov
Copy link

codecov bot commented Oct 26, 2021

Codecov Report

Merging #6889 (799cb54) into main (2bf060c) will decrease coverage by 0.09%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6889      +/-   ##
==========================================
- Coverage   50.69%   50.60%   -0.10%     
==========================================
  Files         126      126              
  Lines        4894     4903       +9     
  Branches     1170     1176       +6     
==========================================
  Hits         2481     2481              
- Misses       2403     2412       +9     
  Partials       10       10              
Impacted Files Coverage Δ
packages/cli/src/util/deploy/process-deployment.ts 10.25% <0.00%> (-1.34%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2bf060c...799cb54. Read the comment docs.


if (event.type === 'checks-conclusion-failed') {
output.stopSpinner();
return event.payload;
Copy link
Member

Choose a reason for hiding this comment

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

Is the payload an error here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's the Deployment. Since even if the checks failed, the Deployment can still succeed, but the aliases won't get assigned.

@@ -88,6 +88,42 @@ export async function* checkDeploymentStatus(
yield { type: 'ready', payload: deploymentUpdate };
}

if (deploymentUpdate.checksState !== undefined) {
if (deploymentUpdate.checksState === 'completed') {
finishedEvents.add('checks-completed');
Copy link
Member

Choose a reason for hiding this comment

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

You're adding to the finishedEvents set but never reading from it.

Seems like it should either check for existence or just remove these lines since the values aren't used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you know what finishedEvents is used for? I just did what all other events do, I though that it's a way to just keep track of which events were already yielded.

Copy link
Member

Choose a reason for hiding this comment

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

I don't recall, but looking at the code it seems to be used to avoid yielding duplicate events

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case it'd be expected to add it to finishedEvents, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I see

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Co-authored-by: Chris <7249920+chriswdmr@users.noreply.github.com>
@AndyBitz AndyBitz merged commit 28e71ff into main Oct 27, 2021
@AndyBitz AndyBitz deleted the cli-and-client-add-checks branch October 27, 2021 18:22
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.

None yet

4 participants