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

Ensure error message for failed slices stay within the slice #13042

Merged
merged 1 commit into from
Sep 16, 2022

Conversation

mads-hartmann
Copy link
Contributor

@mads-hartmann mads-hartmann commented Sep 16, 2022

Description

Background

The Werft log cutter expects each line of output to follow the specific format (docs). If no slice is specified in a log line then it will be attached to the "default" slice for that phase (which is a slice with the same id as the phase).

As we're just console.log'ing strings it means it's out responsibility to ensure we don't log multiline strings.

The change

This PR improves our handling of slices that we explicitly fail using werft.fail in two important ways:

  1. Previously it would not handle multi-line error messages well which meant that the error message for a slice might get split between the slice that failed and the default slice for the phase.
  2. werft.fail throws an error which will eventually bubble up to our main catch handler. In that handler we would console.log the error which meant that the error would always end up in the default slice (see screenshot below)

With this PR any place where we use werft.fail we'll now ensure the error is contained to the slice, which makes it much easier to understand why a slice failed.

Implementation details

I have introduced a new custom error type SliceFailedError which we use in werft.fail. In the main catch handler we then don't perform any additional logging.

Some other notes:

  • it appears that when you use [<slice>|FAIL] <reason> Werft won't display the reason. Because of that I updated the code so the last log output for the slice is "Failed. Expand to see why" and I have left the fail reason blank.
  • You can't fail or done a phase, only slices. I've updated validate-changes.ts so it doesn't try to do so. There are still many other places where we can make similar changes.

Related Issue(s)

No issue. Friday Tid(a)y

How to test

To test this I introduced a TS error in build.ts and side-loaded the error to ensure that the "tsc --noEmit" slice would fail. I did this on this branch and on a branch off main to show the the difference.

werft job run github -s .werft/build.ts

Before (branch off main)

Screenshot 2022-09-16 at 20 54 03

After (this branch)

Screenshot 2022-09-16 at 20 56 02

Release Notes

NONE

Documentation

N/A

Werft options:

  • /werft with-preview

@mads-hartmann mads-hartmann force-pushed the mads/better-error-slicing branch from 3a8bf9e to bd81515 Compare September 16, 2022 18:50
@mads-hartmann mads-hartmann marked this pull request as ready for review September 16, 2022 19:09
@mads-hartmann mads-hartmann requested a review from a team September 16, 2022 19:12
Copy link
Contributor

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

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

Awesome change. Multiline messages getting spread over slices has bothered me since forever!!

@roboquat roboquat merged commit 5bae14d into main Sep 16, 2022
@roboquat roboquat deleted the mads/better-error-slicing branch September 16, 2022 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants