-
Notifications
You must be signed in to change notification settings - Fork 63
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
frontend: provide log-detective explain integration for failed builds #3608
base: main
Are you sure you want to change the base?
Conversation
d51316c
to
b9793b7
Compare
<a href="{{ failed_build_log_url | format(url) }}" class="btn btn-default btn-xs"> | ||
<span class="pficon pficon-info"></span> Explain with Log Detective AI | ||
</a> | ||
{% endif %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we place this into a separate column, since this is not a log file but "request for explanation".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or, maybe we could have a clickable "failed" state? Can you upload a screenshot?
@@ -157,6 +157,12 @@ | |||
<span class="pficon pficon-ok"></span> {{ state }} | |||
{% elif state == "failed" %} | |||
<span class="pficon pficon-error-circle-o"></span> {{ state }} | |||
{% if failed_build_log_url and config.LOG_DETECTIVE_BUTTON %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The used config option is ON in prod, and we want to keep this new UI disabled for some time... I think we need yet another option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe not, it will be quite some time before we'll do a new Copr release anyway.
|
||
Returns: | ||
the full URL to the log file or None if the log file does not exist | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷 this rewrite is weird, can we have a separate method for the new link?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or IMHO even better, can we improve log detective to handle the compressed logs?
Would you mind uploading the screenshot? |
b9793b7
to
42a89fa
Compare
button next to the fail state: or 793ee5b introduces separate column for the table |
42a89fa
to
793ee5b
Compare
793ee5b
to
e079ab3
Compare
IMHO the buttons come out too aggressive. I would probably go with a normal link and an icon (to look the same as the chroot name in the first column). And I would probably shorten the text to something like "Explain failure" or "Explain with AI". |
From the meeting: [x] failed (Ask AI / Teach AI) |
Pull Request validationFailed🔴 Review - Missing review from a member (2 required) Success🟢 CI - All checks have passed |
Merge after fedora-copr/logdetective-website#207