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

Status.tsx: show as many different error types as possible (HMS-1442) #2066

Merged
merged 1 commit into from
May 27, 2024

Conversation

schuellerf
Copy link
Collaborator

As far as I saw error.details rarely contains a .reason again.
It's sometimes directly a string and maybe there is error.details.s or it's even a list of strings
I'm really not sure if this is the right place to handle this but the error.details are somehow support to contain "arbitrary" stuff... (For this uncertainty it's a draft for now)

Also \n does not seem to work with react and <p> so I needed this map()

@schuellerf
Copy link
Collaborator Author

A total crash of osbuild should not happen but if it does it would look like this

NOTE: the forwarding of the last 20 lines of stderr is still to be committed if we want this - but it would look similar for other error details
Screenshot_20240508_110723

@schuellerf schuellerf force-pushed the show-more-error-details-HMS-1442 branch 2 times, most recently from 6728984 to dec120d Compare May 8, 2024 09:17
Copy link
Member

@croissanne croissanne left a comment

Choose a reason for hiding this comment

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

Amazing! thank you

if (error.details?.reason) {
detailsArray.push(`${error.details.reason}`);
}
if (error.details?.s) {
Copy link
Member

Choose a reason for hiding this comment

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

Huh what's the s field?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Update from our chats aside this issue:
It seems to stem from my confusion from the golang error type having the error in .s

https://github.com/golang/go/blob/1843464f014c946c1663de76249267486887626f/src/errors/errors.go#L67

but this seems to never land in the frontend anyway due to
https://github.com/golang/go/blob/1843464f014c946c1663de76249267486887626f/src/builtin/builtin.go#L309

not being able to serialize …
I'll remove from the frontend...

<div className="pf-v5-c-code-block__content">
<pre className="pf-v5-c-code-block__pre">
{detailsArray.join('\n')}
<code className="pf-v5-c-code-block__code"></code>
Copy link
Member

@croissanne croissanne May 8, 2024

Choose a reason for hiding this comment

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

Not sure about using code blocks here, the result doesn't work well for more common errors:

DNF:
image

target errors:

image

We can maybe make the popover wider, and then have reason and details just separated by a vertical line or something? Maybe just details in a codeblock?

If we can somehow tell when it's a stacktrace, putting that in a codeblock could be nice.

Copy link
Collaborator Author

@schuellerf schuellerf May 8, 2024

Choose a reason for hiding this comment

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

totally agree - that's actually for someone else to decide - I'm far away from being a react, PatternFly or design expert!

@kelseamann
Copy link

Hey, I like that we have the english error terminology separated from the code itself. I think it looks great!

@schuellerf schuellerf force-pushed the show-more-error-details-HMS-1442 branch 2 times, most recently from 4d0ab42 to 3528e62 Compare May 21, 2024 09:16
@schuellerf schuellerf marked this pull request as ready for review May 21, 2024 09:16
@schuellerf
Copy link
Collaborator Author

@lucasgarfield what are your suggestions for visual improvements?
content wise this should be a little better than before

@regexowl
Copy link
Collaborator

Awesome! Thanks for looking into this!

Checked out how the error is formatted - would it make sense to extract the reason outside of the code block? If I understand it correctly the reason would be a shorter summary and the possible json/code snippets would be contained in the details.

Would something like this work?
image

      <Popover
        data-testid="errorstatus-popover"
        position="bottom"
        minWidth="40rem"
        bodyContent={
          <>
            <Alert variant="danger" title={text} isInline isPlain />
            <Text className="pf-u-pt-md pf-u-pb-md">{reason}</Text>
            <Panel isScrollable>
              <PanelMain maxHeight="25rem">
                <CodeBlock>
                  <CodeBlockCode>{detailsArray.join('\n')}</CodeBlockCode>
                </CodeBlock>
              </PanelMain>
            </Panel>
            <Button
              variant="link"
              onClick={() =>
                navigator.clipboard.writeText(
                  reason + '\n\n' + detailsArray.join('\n')
                )
              }
              className="pf-u-pl-0 pf-u-mt-md"
            >
              Copy error text to clipboard <CopyIcon />
            </Button>
          </>
        }
      >

(I've set a wider minWidth and moved the things around a bit so only the code part would be scrollable)

@schuellerf schuellerf force-pushed the show-more-error-details-HMS-1442 branch from 3528e62 to d9ad410 Compare May 22, 2024 14:01
@regexowl regexowl force-pushed the show-more-error-details-HMS-1442 branch 2 times, most recently from f9b45f2 to b9a3487 Compare May 23, 2024 09:21
@regexowl
Copy link
Collaborator

/retest

@regexowl regexowl force-pushed the show-more-error-details-HMS-1442 branch from b9a3487 to 7be1da0 Compare May 27, 2024 07:42
@regexowl
Copy link
Collaborator

/retest

1 similar comment
@regexowl
Copy link
Collaborator

/retest

Copy link
Collaborator

@regexowl regexowl left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!

@regexowl regexowl merged commit 77fd8d6 into osbuild:main May 27, 2024
5 checks passed
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.

5 participants