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

Parse evaluation warnings and display in separate section. #180

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MagicRB
Copy link
Contributor

@MagicRB MagicRB commented Jun 12, 2024

A live version can be seen here and in case my buildbot dies or gets reset, a screenshot:

image

Possibly fixes #172, for anything more fancy we'll need #173, what do you think @zimbatm?

And due to the exploration here, I've learned that we can add arbitrary HTML to the page, as long as it is contained in one of those tabs. Well, injecting javascript and modifying the page is an option, but a really bad one :)

Copy link
Member

@zimbatm zimbatm left a comment

Choose a reason for hiding this comment

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

nice, this is on the right track

@@ -172,6 +172,11 @@ def run(self) -> Generator[Any, object, Any]:
# if the command passes extract the list of stages
result = cmd.results()
if result == util.SUCCESS:
log.info(self.observer.getStderr())
self.addHTMLLog(
"Evaluation Warnings", f"<pre>{self.observer.getStderr()}</pre>"
Copy link
Member

Choose a reason for hiding this comment

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

Do you know if the string interpolation gets properly HTML-escaped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uh, definitely not, good catch! JavaScript injection attacks disaprove :)

Copy link
Member

Choose a reason for hiding this comment

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

I think the python stdlib has some helpers for that. Otherwise I am sure that buildbot has it somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, but this was more of a prototype to showcase what can be done now anyway, but i don't think it's worth merging as it is, personally.

@@ -172,6 +172,11 @@ def run(self) -> Generator[Any, object, Any]:
# if the command passes extract the list of stages
result = cmd.results()
if result == util.SUCCESS:
log.info(self.observer.getStderr())
Copy link
Member

Choose a reason for hiding this comment

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

Could you make the eval errors more visible? As a user, I would see this when looking at the build:

image

Then click on two toggles to unveil the eval error:

image

(small nit: the whitespace in front of the first line)

Copy link
Contributor Author

@MagicRB MagicRB Jun 12, 2024

Choose a reason for hiding this comment

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

Whitespace is already fixed, left over from a test and me not knowing how pre works. But i dont think we can autoexpand, ill look. But to not put it there at all, would need UI changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah i looked, we cant autoexpand without hacks. There is API to edit the UI, only to replace it :/

Copy link
Member

Choose a reason for hiding this comment

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

What about the
image
Could this be turned orange somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will check, but I dont have high hopes

Copy link
Contributor Author

@MagicRB MagicRB Jun 13, 2024

Choose a reason for hiding this comment

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

Builbots UI is very configurable, if you want to add/replace views, not if you want to modify them... i'll join their IRC/something and ask, in case I missed something

@MagicRB
Copy link
Contributor Author

MagicRB commented Jun 12, 2024

Anything more than this is blocked by #173 and further discussion is needed, as buildbot does not allow partial UI replacements as far as I can tell, therefore we cannot add another element into the build UI.

@zimbatm
Copy link
Member

zimbatm commented Jun 13, 2024

Unless the eval error gets more visibility, it makes sense to table this for now.

@Mic92
Copy link
Member

Mic92 commented Jun 13, 2024

@mergify rebase

Copy link

mergify bot commented Jun 13, 2024

rebase

✅ Branch has been successfully rebased

@Mic92 Mic92 force-pushed the parse_evaluation_warnings branch from 98c63d8 to 1788b41 Compare June 13, 2024 09:07
@MagicRB
Copy link
Contributor Author

MagicRB commented Sep 11, 2024

This is superseded by #255 as now evaluation failures appear as separate builds.
image

@MagicRB MagicRB closed this Sep 11, 2024
@Mic92
Copy link
Member

Mic92 commented Sep 21, 2024

But is this the same as warnings though?

@Mic92 Mic92 reopened this Sep 21, 2024
@Mic92
Copy link
Member

Mic92 commented Sep 21, 2024

But is this the same as warnings though? Those

@Mic92 Mic92 closed this Sep 21, 2024
@MagicRB
Copy link
Contributor Author

MagicRB commented Sep 21, 2024

Ah yeah if the build doesn't fail it doesn't show. That's my mistake.

@MagicRB MagicRB reopened this Sep 21, 2024
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.

Parse the Nix evaluation warnings and display them in the UI
3 participants